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

Optimize sprite.c #3175

Merged
merged 3 commits into from
Aug 3, 2023
Merged

Optimize sprite.c #3175

merged 3 commits into from
Aug 3, 2023

Conversation

mrgriffin
Copy link
Collaborator

@mrgriffin mrgriffin commented Jul 25, 2023 •
edited
Loading

Optimizes BuildOamBuffer:

  • Only sorts visible sprites.
  • Uses quicksort. Uses insertion sort with minimal writes for better performance on mostly-sorted data (which sprites typically are).
  • Sorts on a single u32 key which is computed up-front.
  • Eliminates sSpriteOrder and sSpritePriorities and the replacement lives in IWRAM (on the stack).
  • Only copies gDummyOamData to OAM slots that aren't already dummied.
  • Only copies matrices that are in use.

mrgriffin added 2 commits July 24, 2023 15:18
@mrgriffin
Add BENCHMARK to test runner
3f5daa8 
Counts the time spent in the block in arbitrary time units (64-cycles
currently). If the block takes more than ~4 million cycles (~0.25s) the
benchmark will wrap around.

Note that the time can be affected by the timeout IRQs, and should only
be taken as a loose indication of relative performance.
@mrgriffin
Print ticks
b499f41
@mrgriffin mrgriffin marked this pull request as ready for review July 25, 2023 09:49
@mrgriffin mrgriffin force-pushed the rhh-optimize-sprite branch from 736349f to 1351524 Compare July 25, 2023 09:49
@mrgriffin
Copy link
Collaborator Author

Note, this PR contains #3172.

DizzyEggg
DizzyEggg requested changes Jul 25, 2023
View reviewed changes
Copy link
Collaborator

@DizzyEggg DizzyEggg left a comment

Choose a reason for hiding this comment

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

Bad news 😬

The game softlocks randomly during the intro.

@mrgriffin
Copy link
Collaborator Author

I think I found the culprit—I had a u32 but should have used a s32.

@DizzyEggg
Copy link
Collaborator

I think I found the culprit—I had a u32 but should have used a s32.

Yeah, that must've been it. The game plays fine now

@AsparagusEduardo
Copy link
Collaborator

Note, this PR contains #3172.

Is that PR ready to review then? Or will you be replacing it with this one?

@mrgriffin
Copy link
Collaborator Author

Note, this PR contains #3172.

Is that PR ready to review then? Or will you be replacing it with this one?

Whoops! Yeah, I've marked it as ready. I had it draft just in case anything came up with this PR, but I didn't make any changes :)

@AsparagusEduardo
Copy link
Collaborator

Then I would mark this PR as draft until that one is approved.

DizzyEggg
DizzyEggg previously approved these changes Jul 26, 2023
View reviewed changes
@DizzyEggg DizzyEggg self-requested a review July 26, 2023 07:50
SBird1337
SBird1337 requested changes Jul 27, 2023
View reviewed changes
Copy link
Collaborator

@SBird1337 SBird1337 left a comment

Choose a reason for hiding this comment

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

In addition I compared the implementation in a somewhat non-scientific way to the implementation ipatix made for sots, and it is pretty comparable speed-wise for the small toy examples I played with :)

gflib/sprite.c Outdated Show resolved Hide resolved
gflib/sprite.c Show resolved Hide resolved
gflib/sprite.h Outdated Show resolved Hide resolved
@SBird1337
Copy link
Collaborator

I think once this PR gets approved we can just have it supersede #3172 - alternatively we can merge that and you can rebase if you think it is useful to have that logical distincion :)

@mrgriffin
Copy link
Collaborator Author

tbh I don't think the logical distinction is that important, I'll just rebase this branch once I've applied your suggestions. I'll aim to do that next week, I'm very busy with work for the coming few days.

@mrgriffin mrgriffin mentioned this pull request Jul 30, 2023
Closed
@mrgriffin mrgriffin force-pushed the rhh-optimize-sprite branch from 2745053 to b08b6a3 Compare August 2, 2023 11:22
@mrgriffin
Copy link
Collaborator Author

Addressed review comments, rebased, and pushed :)

@SBird1337 SBird1337 merged commit 7d8bcce into rh-hideout:upcoming Aug 3, 2023
@AsparagusEduardo AsparagusEduardo mentioned this pull request Sep 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DizzyEggg DizzyEggg DizzyEggg approved these changes

@SBird1337 SBird1337 SBird1337 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@mrgriffin @DizzyEggg @AsparagusEduardo @SBird1337