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

Removed superfluous code to determine the number of compressed bones. #991

Merged
merged 3 commits into from Apr 6, 2019

Conversation

Projects
None yet
3 participants
@AJSchat
Copy link
Contributor

commented Oct 31, 2018

The compressed bone pool of a Ghoul II animation file (MDXA, .gla) is always
stored at the end of the file. It is unnecessary to iterate through all frames
to determine the number of compressed bones present on big-endian machines.

This change is applied to the vanilla renderers of the single- and multiplayer
code of JK:JA.

Removed superfluous code to determine the number of compressed bones.
The compressed bone pool of a Ghoul II animation file (MDXA, .gla) is always
stored at the end of the file. It is unnecessary to iterate through all frames
to determine the number of compressed bones present on big-endian machines.

This change is applied to the vanilla renderers of the single- and multiplayer
code of JK:JA.
@xycaleth

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Thanks for the PR @AJSchat.

My main concern with your change is if anybody wants to change the Ghoul2 format in their fork then they will need to know to change this code too - not doing so would cause the wrong number of bones to be calculated. So, it's not safe to assume that the bone pool will always reside at the end of the file.

Re-added code to determine the number of compressed bones.
The original code removed in commit b993a8c is now only used when dealing with
MDXA formats different than the original format (version 6) present in JK:JA.
This ensures the compressed bone count is always calculated properly.
@AJSchat

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

Thanks for your prompt reply xycaleth!

I have just created another commit which only applies the new code to version six of the MDXA format. For any other version it falls back to the original behavior attempting to find the maximum bone index by iterating through all bones across frames. It should be safe to assume that anyone wishing to extend the Ghoul II MDXA file format will alter the internal version number too, right? Or do you have a different opinion regarding the matter?

I would also like to discuss altering the mdxaIndex_t structure. Right now, iIndex is an integer where the last byte is masked to match the 3 byte indices present in the MDXA files. I would like to alter the iIndex variable type from an integer to a three byte array, so any function using the indexes can convert the three bytes to an integer without having to worry about endianness. What happens on big-endian machines right now is that the last byte is masked, then byte swapped and returned for every time a bone is decompressed. Converting the three bytes to an integer directly is more clear, faster on big-endian machines and is endian independent.

Please let me know if you agree with this change so I can also add that change to this pull request.

@Archangel35757

This comment has been minimized.

Copy link

commented Nov 2, 2018

@AJChat -- DF2 mod (and others) would like to make some improvements to the Ghoul2 format (calling it Ghoul3 and updating the header)-- especially regarding the bone compression and also switch from global transforms to parent-local space transforms... all in an effort to eliminate quantization/ bone jitter in long bone chains. you should join the OpenJK Discord channel so you can see what improvements were suggested.

@xycaleth

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

@Archangel35757 this isn't the place to advertise your mod...

Thanks for your prompt reply xycaleth!

No problem :)

I have just created another commit which only applies the new code to version six of the MDXA format. For any other version it falls back to the original behavior attempting to find the maximum bone index by iterating through all bones across frames. It should be safe to assume that anyone wishing to extend the Ghoul II MDXA file format will alter the internal version number too, right? Or do you have a different opinion regarding the matter?

I don't feel like the change here is necessary. The code for the change is fine, but why complicate the code for an insignificant performance improvement? + it's kind of weird to me to check for a specific version when loading.

As far as I know, the code here isn't a bottleneck and .gla files are very rarely loaded, so I don't think any change here is needed.

I would also like to discuss altering the mdxaIndex_t structure. [...] Converting the three bytes to an integer directly is more clear, faster on big-endian machines and is endian independent.

I'm all for making the code clearer to read so this sounds good to me 👍

@AJSchat

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

I can see where you're coming from. The change is indeed unnecessary when taking future expandability of the Ghoul II format in account and becomes even messier checking for versions of the format. I suppose the change would make more sense if OpenJK's goal wasn't to provide a clean base for other mods to build on.

I will revert the changes made to the MDXA loading functions and commit the other small change to the format in the coming days.

Changed the mdxaIndex_t struct.
It now contains a three byte array rather than a regular integer. This has been
changed so the G2_GetBonePoolIndex function (and any future implementation) can
convert the three byte integer at such an offset without having to worry about
endianness.
@AJSchat

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

I just reverted the change and altered the mdxaIndex_t struct as discussed earlier.

Please let me know if any other change is required here or if the PR is approved or rejected as-is. Thank you in advance.

@xycaleth xycaleth merged commit 212d7d8 into JACoders:master Apr 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.