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

Change the way GLTFDocumentExtension classes are registered #66026

Merged
merged 1 commit into from
Nov 20, 2022

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 18, 2022

This PR changes the way GLTFDocumentExtension classes are registered. Previously the single extension was registered in GLTFDocument's constructor, which is not a good approach. This PR allows using GLTFDocumentExtension from GDScript or GDExtension unlike the old approach which only worked well for code directly in core.

There is now a static Vector all_document_extensions, and there is a method to add to this. This allows registering GLTFDocumentExtensions that can be used. Whether a GLTFDocumentExtension is actually used for a particular file import depends on the result of import_preflight, if it returns an error then the document extension will not be used (if it returns OK, it will be used, and we add the document extension to a non-static Vector per each instance of GLTFDocument).

All GLTFDocumentExtension classes are assumed to be stateless.

This work was sponsored by The Mirror.

@aaronfranke aaronfranke added this to the 4.0 milestone Sep 18, 2022
@aaronfranke aaronfranke requested a review from fire September 18, 2022 03:34
@aaronfranke aaronfranke requested a review from a team as a code owner September 18, 2022 03:34
@fire
Copy link
Member

fire commented Sep 18, 2022

We designed this in conversation in discord. Will review. Maybe @lyuma

@fire fire requested a review from a team September 18, 2022 04:32
@aaronfranke aaronfranke force-pushed the gltf-extension branch 2 times, most recently from 9dd1e02 to be0eada Compare September 19, 2022 03:09
@fire
Copy link
Member

fire commented Sep 19, 2022

We had further review with @lyuma and @aaronfranke today.

Decisions discussed in newest order first.

  1. GLTFDocumentExtension should return a list of extensions implemented
    and then the base importer can check that all required extensions were properly implemented
  2. Well right now the method returns an Error code.
    We could make it so that if import_preflight returns an error code then we don't enable it.
  3. state->json["extensions"] should be state->json["extensionsUsed"]
  4. iirc GLTFState only has the "generated" GLTF path, not the original file's. Discussed. iFire mentioned that glb buffers don't have paths.
  5. move out the first commit the lights and n
  6. Memory hazy..

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 19, 2022

  1. Is done, the method is called get_supported_extensions, it's in the last commit. EDIT: This part was merged in GLTF: Store extensions in GLTFState and get supported extensions from GLTFDocumentExtension #66094
  2. Is done, if import_preflight returns an error code the extension will not be used. Same with export_preflight.
  3. Is taken care of, I ensured that extensionsUsed is used everywhere it's supposed to.
  4. Is not a part of this PR.
  5. Is done, the PR is Minor enhancements to the GLTF module (lights and docs) #66087 (this is actually merged but GitHub bugged out and it broke)
  6. I hope your memory becomes less hazy :)

EDIT: I also opened #66094 which is another subset of this PR.

@fire
Copy link
Member

fire commented Sep 20, 2022

The starting message is now out of date and the pr needs rebase. Working on #66094

@fire
Copy link
Member

fire commented Oct 3, 2022

I wasn't able to work on this this weekend. I'll see if I can get someone from our group to check.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Thanks for splitting. It makes review better.

@@ -44,13 +45,13 @@

class GLTFDocument : public Resource {
GDCLASS(GLTFDocument, Resource);
TypedArray<GLTFDocumentExtension> document_extensions;
static Vector<Ref<GLTFDocumentExtension>> all_document_extensions;
Vector<Ref<GLTFDocumentExtension>> document_extensions;
Copy link
Member

@fire fire Oct 18, 2022

Choose a reason for hiding this comment

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

We devised this way to implement the two lists of extensions. Active vs all.

@lyuma
Copy link
Contributor

lyuma commented Oct 21, 2022

I won't be the one to block this review over this since I'm not familiar enough with the usage of C++ elsewhere, but I would like to warn you against the use of a static Vector.

__static_initialization_and_destruction is not something I like to deal with. Prepare for Unforseen Consequences.

The Godot pattern that I would suggest to use is to have a GLTFExtensionRegistry singleton, which is initialized in module init and destructed in module uninit, avoiding the use of complex static objects.

@fire
Copy link
Member

fire commented Oct 21, 2022

@lyuma 's method is more robust. Can you investigate @aaronfranke ?

@aaronfranke
Copy link
Member Author

@fire @lyuma I'm not familiar enough with that to be confident in any changes. If there is a better approach I'd want feedback from a core Godot dev like @reduz or @akien-mga before I do potentially unnecessary work that I'm unfamiliar with. Also,static Vector works fine in this PR, so we could merge as-is and it could be modified further in a future PR.

@aaronfranke aaronfranke force-pushed the gltf-extension branch 2 times, most recently from 4086127 to 196c8cc Compare November 3, 2022 02:48
@fire
Copy link
Member

fire commented Nov 3, 2022

I apologize for the delay, it's on reduz's review list.

@fire fire requested a review from a team November 3, 2022 14:40
Also move GLTFDocumentExtension into the extensions folder
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I think this is good. We've waited an extended period and reduced the scope.

@akien-mga akien-mga merged commit d5d83ee into godotengine:master Nov 20, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants