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.

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

Fixes july #287

Merged
merged 13 commits into from
Jul 8, 2020
Merged

Fixes july #287

merged 13 commits into from
Jul 8, 2020

Conversation

KrahJohlito
Copy link
Member

@KrahJohlito KrahJohlito commented Jul 6, 2020

Pull Request checklist

Note: these are not necessarily requirements

  • I reformatted the code with clang-format
  • I checked to make sure my submission worked
  • I am the author of submission or have permission from the original author
  • Requires update of the PS2SDK
  • Requires update of the gsKit
  • Others (please specify below)

Pull Request description

Take two on this.. same as before but this time I've also switched all use of clock() to ps2_clock() since it works without issues, and fixed the Last Played Auto Start counter now also.

Deferred calls are already in use in current builds so I see no issue this going ahead due to them, they could possibly be looked at later when more time is available.

As for the loading icon.. we could only display it after gInitComplete to avoid showing the default icon and the custom theme icon switch.. but some people may not like this as it means you don't see any icon until all device etc are loaded.. so I have just left it for now.

@uyjulian uyjulian mentioned this pull request Jul 7, 2020
6 tasks
@rickgaiser
Copy link
Member

Please revert "switch from clock to ps2_clock function". The issue with clock() should now be solved in ps2sdk. Can you verify the issues are gone with the updated clock function? Next time please try to find the root-cause of issues instead of working your way around them.

The commits in this PR both change things and then revert some of the same things. I think these should be squashed before creating the PR, but I understand these git operations are not easy for everyone. Instead I suggest we squash-merge this PR if it gets pulled/approved.

Also try to be more descriptive of your PR's. Becouse "Fixes july" does not tell me anything about this PR. What does this fix? What was broke? The description also does not describe this.
Instead something like "Improve time duration calculation for sound and transitions" would more describe the purpose of this PR.
If there's really "fixes" for bugs in this PR then these would be best put in a separate PR. As they are often small and pulled fast.

@KrahJohlito
Copy link
Member Author

Please revert "switch from clock to ps2_clock function". The issue with clock() should now be solved in ps2sdk. Can you verify the issues are gone with the updated clock function? Next time please try to find the root-cause of issues instead of working your way around them.
I haven’t tested yet, due to my Timezone by the time I made the PR I went to bed immediately to be ready for work.. which I am still at.. I will test after work and rl commitments.

Also I am not a professional programmer.. I’ve been bangin on about clock() for over a week now; there is no way I would’ve been able to find the cause.. only steer someone else in the right direction, which I tried to do only to have you mocking me each time.

Also try to be more descriptive of your PR's. Becouse "Fixes july" does not tell me anything about this PR. What does this fix? What was broke? The description also does not describe this. Instead something like "Improve time duration calculation for sound and transitions" would more describe the purpose of this PR. If there's really "fixes" for bugs in this PR then these would be best put in a separate PR. As they are often small and pulled fast.
I would’ve thought the commit messages were self explanatory but fair enough.

You appear to have some sort of chip on your shoulder in regards to me, I suggest you get over whatever issue you have as at this point I just find the shit comical.

@KrahJohlito
Copy link
Member Author

done.. this: ps2dev/ps2sdk#133 does indeed fix all issues related to the clock().. thank you for taking the time to look at it..
PR is finished..squash or do whatever you want with it.. I will not being contributing any further, I only hope that I was some help in moving towards a stable build.. as that was my goal.

@rickgaiser
Copy link
Member

Also I am not a professional programmer.. I’ve been bangin on about clock() for over a week now; there is no way I would’ve been able to find the cause.. only steer someone else in the right direction, which I tried to do only to have you mocking me each time.

It's perfectly fine if you're not a professional programmer, as long as the quality of the end result is acceptable. I'm always trying to be constructive towards everyone. I'm not just telling you something is bad. I'm also telling you why and how it can be done differently. I'm not 'mocking' you each time, but I'm trying to help, each time. I hope you and others learn from it, and if I'm wrong in my comments then I hope you'll tell me otherwise and I can learn from you.

You appear to have some sort of chip on your shoulder in regards to me, I suggest you get over whatever issue you have as at this point I just find the shit comical.

The only problem I have recently is a lack of time. So instead of fixing problems like these I'm hoping others, like you, can find the root-couse of this issue and fix it. Working around issues will not make the build more stable. Becouse the issues are probably still there. Although in this case you would have worked around the issue by not using clock anymore, other applications would still be broke.

I will not being contributing any further, I only hope that I was some help in moving towards a stable build.. as that was my goal.

I'm sorry to hear that, I hope you change your mind.

PS: I don't want to mock you again, but I do still intend to review the code.

Copy link
Member

@rickgaiser rickgaiser left a comment

Choose a reason for hiding this comment

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

This looks ok to me. I have not done any runtime tests, so if someone can do that I think it's good to go.
Build can be downloaded here:
https://github.com/KrahJohlito/Open-PS2-Loader/suites/885397230/artifacts/10457707

@ElPatas1
Copy link
Contributor

ElPatas1 commented Jul 8, 2020

@KrahJohlito, @rickgaiser, thank you very much for the work, I and @J013k we tested this test build 1519,
the exit program do not freezes and all looks fine, great.

But we have a doubt, if the fix of "clock() function" is moved recently to the PS2DK,
Does this 1519 version has it own fix or it was compiled with fixed PS2SDK?

Also, it is not needed update our PS2SDK for compile the OPL?

Best regards.

EDIT: Well it is answered in psx-place that yes that this 1519 build have the fix from PS2SDK.
I merge this commits.

@ElPatas1
Copy link
Contributor

ElPatas1 commented Jul 8, 2020

@rickgaiser i have a doubt, why you suggest squash and merge this PR, instead the standard merge?

There it is a lot of commits, 11, wouldn't it be better to merge them all separately so that each one of them can be well
identified in the future, instead of merging them all in a single commit with the squash and merge option?

Best regards.

@rickgaiser
Copy link
Member

Within this 1 PR the same lines are changed multiple times. The separate commits are not usable/finished, but the end result is. That's why I suggested the squash, so these multiple changes are squashed into 1. So in this case I think it's cleaner to squash them into 1, but on the other hand you do loose some interesting commit information.

I'm not sure in this case what's best, I have doubts as well. Do whatever you think is best ;).

@ElPatas1
Copy link
Contributor

ElPatas1 commented Jul 8, 2020

@rickgaiser, mmm... well, i'm going to bet on how we did so far, use the standard merge so that they
can be identified and that interesting information is not lost.

Best regards.

@ElPatas1 ElPatas1 merged commit 82cc4bf into ps2homebrew:master Jul 8, 2020
@ElPatas1 ElPatas1 mentioned this pull request Jul 8, 2020
@KrahJohlito KrahJohlito deleted the fixes-july branch July 19, 2020 10:47
AKuHAK pushed a commit that referenced this pull request Sep 30, 2021
citronalco pushed a commit to citronalco/OPL-Daily-Builds that referenced this pull request Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants