Skip to content

Rewrite frame CRC generation#1369

Open
Starkku wants to merge 7 commits intoPhobos-developers:developfrom
Starkku:feature/framecrc
Open

Rewrite frame CRC generation#1369
Starkku wants to merge 7 commits intoPhobos-developers:developfrom
Starkku:feature/framecrc

Conversation

@Starkku
Copy link
Copy Markdown
Contributor

@Starkku Starkku commented Aug 30, 2024

Reimplements game's frame CRC generation function, only change logic-wise is that it now excludes AnimClass and ParticleClass objects that are purely visual.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 30, 2024

Nightly build for this pull request:

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

@FS-21
Copy link
Copy Markdown
Contributor

FS-21 commented Aug 30, 2024

What kind of tests should be done here if you want feedback?

Copy link
Copy Markdown
Contributor

@chaserli chaserli left a comment

Choose a reason for hiding this comment

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

If you're rewriting these completely, it would be better to hash these in a more modern way for better readability and extensibility

Comment thread src/Misc/SyncLogging.cpp Outdated
Comment thread src/Misc/SyncLogging.cpp Outdated
Comment thread src/Misc/SyncLogging.cpp Outdated
Comment thread src/Misc/SyncLogging.cpp Outdated
Comment thread src/Misc/SyncLogging.cpp Outdated
Comment thread src/Misc/SyncLogging.cpp Outdated
@Starkku
Copy link
Copy Markdown
Contributor Author

Starkku commented Sep 15, 2024

The wrapper may be good idea in theory but that implementation mostly just obfuscates what gets hashed and for what type of object (it also changes the functionality from how game does it slightly, although likely to little to no tangible effect). The benefits would be more obvious if there was a realistic chance for new properties that need hashing to be added for, say, TechnoClass and derivatives in the future but as it stands this is very unlikely. If you can think of a way to do it without extra overhead or the obfuscating nature while reducing repeated code be my guest, but the previous implementation was not it so I've elected to revert it. I do hold a local copy of it for now still just in case though.

I'm not even going to repeat the point about the rules hash check in Phobos anymore, should be obvious by now that it does not belong here.

I did do some cleanup to the first iteration too.

Comment thread docs/Whats-New.md Outdated
@chaserli
Copy link
Copy Markdown
Contributor

chaserli commented Sep 19, 2024

If you can think of a way to do it without extra overhead or the obfuscating nature while reducing repeated code be my guest

It only wrapped a DWORD and serves for hash the object on creation and do the hash combine operation. I bet it makes less overhead than your current method. It also allows you to know the "sum" at compile time. But anyway these are small things
and make literally no difference.

@Starkku
Copy link
Copy Markdown
Contributor Author

Starkku commented Mar 28, 2025

Rebased on develop and applied some of the feedback. This is ready for further testing and another review pass.

@Coronia
Copy link
Copy Markdown
Contributor

Coronia commented Apr 8, 2025

no desync or other issue occured in our testing so far

@Coronia Coronia added Tested ⚙️T1 T1 maintainer review is sufficient and removed Needs testing labels Apr 8, 2025
@Starkku Starkku force-pushed the feature/framecrc branch 2 times, most recently from 9fb53b2 to fdb27bd Compare May 2, 2025 09:44
@Starkku Starkku force-pushed the feature/framecrc branch 4 times, most recently from 7a87b77 to c9bc0f2 Compare June 13, 2025 12:04
@Starkku Starkku force-pushed the develop branch 2 times, most recently from b429215 to 280b1c8 Compare June 29, 2025 19:13
@Starkku Starkku force-pushed the feature/framecrc branch from c9bc0f2 to ffbda85 Compare July 22, 2025 22:46
@Starkku Starkku force-pushed the feature/framecrc branch 2 times, most recently from b4473e6 to c89b048 Compare December 12, 2025 21:32
@Starkku Starkku requested review from Metadorius and ZivDero April 27, 2026 16:55
Copy link
Copy Markdown
Contributor

@ZivDero ZivDero left a comment

Choose a reason for hiding this comment

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

A couple nitpicks but looks good, see no issues.

Comment thread src/Misc/SyncLogging.cpp Outdated
Comment thread src/Misc/SyncLogging.cpp Outdated
Starkku added 7 commits April 29, 2026 19:31
- Use DEFINE_JUMP to replace calls to ComputeGameCRC instead of hooking in it
- Separate the CRC calculations into inlined functions
- Do not inline the hash eligibility check function due to its size, rename it for clarity
- Misc. minor improvements
@Starkku Starkku added the Will be merged in 24h This PR will be merged in 24 hours if no one has further instructions. label Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚙️T1 T1 maintainer review is sufficient Tested Will be merged in 24h This PR will be merged in 24 hours if no one has further instructions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants