Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor optimize, code refactor and C++17 make_unique #2097

Closed
wants to merge 1 commit into from
Closed

Minor optimize, code refactor and C++17 make_unique #2097

wants to merge 1 commit into from

Conversation

GermanAizek
Copy link
Contributor

@Macdu Hi, review PR changes for readability and minor optimization.
I think you'll like it.

Macdu
Macdu reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@Macdu Macdu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice, thank you! Only a few small things to modify.

vita3k/renderer/src/texture_cache.cpp Outdated
@@ -505,6 +505,7 @@ void upload_bound_texture(const TextureCacheState &cache, const SceGxmTexture &g
case SCE_GXM_TEXTURE_CUBE_ARBITRARY:
width = next_power_of_two(width);
height = next_power_of_two(height);
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This break is not needed and will break the graphics of some games (if the texture type is arbitrary adjust the width and the height, then do like the others).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Macdu, fix it 2fe6d68

vita3k/gui/src/gui.cpp Outdated
@@ -534,7 +534,7 @@ void get_user_apps_title(GuiState &gui, EmuEnvState &emuenv) {

void get_sys_apps_title(GuiState &gui, EmuEnvState &emuenv) {
gui.app_selector.sys_apps.clear();
const std::vector<std::string> sys_apps_list = { "NPXS10003", "NPXS10008", "NPXS10015", "NPXS10026" };
const std::array<std::string, 4> sys_apps_list = { "NPXS10003", "NPXS10008", "NPXS10015", "NPXS10026" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can even make this constexpr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is proleme with vector ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Macdu, fix it 2fe6d68

Zangetsu38
Zangetsu38 suggested changes Sep 29, 2022
View reviewed changes
vita3k/renderer/src/texture_cache.cpp Outdated Show resolved Hide resolved
@GermanAizek
Copy link
Contributor Author

@Zangetsu38 fix it 4ea6b52

Zangetsu38
Zangetsu38 previously approved these changes Oct 3, 2022
View reviewed changes
@Zangetsu38 Zangetsu38 self-requested a review October 3, 2022 08:04
Macdu
Macdu previously approved these changes Oct 8, 2022
View reviewed changes
@Macdu
Copy link
Contributor

Macdu commented Oct 8, 2022

LGTM

bookmist
bookmist previously approved these changes Nov 16, 2022
View reviewed changes
@bookmist
Copy link
Contributor

Please, rebase it on master.

@GermanAizek
Copy link
Contributor Author

Please, rebase it on master.

@bookmist, fix it here fc990bf

@bookmist
Copy link
Contributor

bookmist commented Nov 28, 2022 •
edited

Sorry, I did not explain. I mean master of this repo. Commands to do it are something like this (not checked)

git remote add upstream https://github.com/Vita3K/Vita3K.git
echo fetch upstream master 
git fetch upstream master
echo create local branch with upstream master called upstream_master
git branch upstream_master FETCH_HEAD
echo then rebase your master onto upstream_master. if you use tortoiseGit then command will be
echo TortoiseGitProc.exe /command:rebase /upstream upstream_master
echo then force push```

MickaelFleurus
MickaelFleurus reviewed Jan 17, 2024
View reviewed changes
vita3k/gui/src/gui.cpp
for (const auto &app : sys_apps_list) {
vfs::FileBuffer params;
if (vfs::read_file(VitaIoDevice::vs0, params, emuenv.pref_path, "app/" + app + "/sce_sys/param.sfo")) {
if (vfs::read_file(VitaIoDevice::vs0, params, emuenv.pref_path, std::string("app/") + app + "/sce_sys/param.sfo")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::string(...) specified for app/ but not for /sce_sys/param.sfo?

MickaelFleurus
MickaelFleurus reviewed Jan 17, 2024
View reviewed changes
vita3k/io/include/io/util.h
@@ -46,7 +46,7 @@ struct FileInfo {
int access_mode;

FileInfo()
: vita_loc("")
: vita_loc()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed at all if empty.

@nishinji nishinji mentioned this pull request Apr 15, 2024
Merged
@EXtremeExploit
Copy link
Member

#3274 already merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xerpi xerpi xerpi left review comments

@MickaelFleurus MickaelFleurus MickaelFleurus left review comments

@EXtremeExploit EXtremeExploit EXtremeExploit left review comments

@Macdu Macdu Macdu left review comments

@bookmist bookmist bookmist left review comments

@Zangetsu38 Zangetsu38 Awaiting requested review from Zangetsu38

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@GermanAizek @Macdu @bookmist @EXtremeExploit @xerpi @Zangetsu38 @MickaelFleurus