Skip to content

Conversation

@DethRaid
Copy link
Contributor

@DethRaid DethRaid commented Apr 29, 2025

Adds support for the draft KHR_physics_rigid_bodies extension, and KHR_implicit_shapes

The import code is decently well-tested and used in my game, the export code is a bit rougher

Copy link
Owner

@spnda spnda left a comment

Choose a reason for hiding this comment

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

First of all big thanks. Don't have much to say yet, but one thing I will mention is that the if/else/for style is wildly inconsistent, most often with no space after an if/for, and sometimes with a newline before an else. While I generally don't care about the style contributors use (you'll find a few different styles throughout the codebase), I'd still like the individual commits/PRs to be as consistent as possible, and at best stick to the general codestyle.

Copy link
Owner

@spnda spnda left a comment

Choose a reason for hiding this comment

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

Went over this quickly, most of it looks good. I'll do an in depth review when you have added some testing code, so I can test locally too. Perhaps I should really add some kind of fuzzer, since I'd assume there's only a handful of available assets with this extension.

Well apart from the comments I'd like to mention that your code doesn't use FASTGLTF_UNLIKELY and FASTGLTF_LIKELY much (except for the parts I guess you copy & pasted). Might look annoying, but I found them to make parsing a few percents quicker due to better codegen, in the cases where no errors are returned.

Also for the exporting code (sorry you had to go through that mess), single characters should always be 'x' and not a string.

@DethRaid DethRaid marked this pull request as ready for review May 1, 2025 21:04
Copy link
Owner

@spnda spnda left a comment

Choose a reason for hiding this comment

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

  • I know the usage of likely/unlikely is sometimes wrong in my code, too, but it would be best to get it right here.
  • There's a few issues around indentation, specifically inconsistency between tabs and spaces. I want to slowly switch to using tabs, without creating a commit changing thousands of lines.
  • There's probably still a few things I missed, but the fact the tests succeed everywhere is a good sign. Will need to review the tests in a bit more detail soon, so I know that everything is covered.

@spnda spnda added enhancement New feature or request new-ext Adds a new glTF extension labels May 5, 2025
@spnda spnda merged commit 2fd6026 into spnda:main May 5, 2025
18 checks passed
@spnda spnda added the done label May 5, 2025
@DethRaid DethRaid deleted the KHR_physics_rigid_bodies branch May 5, 2025 23:08
@spnda spnda changed the title Khr physics rigid bodies Add KHR_physics_rigid bodies May 5, 2025
@spnda spnda changed the title Add KHR_physics_rigid bodies Add KHR_physics_rigidbodies May 5, 2025
@spnda spnda changed the title Add KHR_physics_rigidbodies Add KHR_physics_rigid_bodies May 5, 2025
@spnda spnda mentioned this pull request May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request new-ext Adds a new glTF extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants