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

Enable a gltf_gltf2_hook #919

Merged
merged 3 commits into from
Mar 12, 2020
Merged

Enable a gltf_gltf2_hook #919

merged 3 commits into from
Mar 12, 2020

Conversation

jjcasmar
Copy link
Contributor

@jjcasmar jjcasmar commented Feb 10, 2020

This patch allows the user to create a gltf2_hook that returns a new list of scenes and animations. The purpose of this is to allow a second pass that can do more things than what currently the exporter does.

As an example, the exporter checks if an object in bpy.data.objects is exported, and in that case, it checks if it has animations. If it has animations, it exports the animation.

If you want to use glTF2 as an animation library, its not possible, because the animations wont be exported. EXT_property_animation (although not ratified yet), can't be implemented neither as you cant export material animations.

With a second pass, we allow this use cases and more.

@scurest @emackey @julienduroure

@scurest
Copy link
Contributor

scurest commented Feb 10, 2020

I think you could simplify and generalize it by just adding this to __gather_gltf

 def __gather_gltf(exporter, export_settings):
     active_scene_idx, scenes, animations = gltf2_blender_gather.gather_gltf2(export_settings)
 
+    plan = {'active_scene_idx': active_scene_idx, 'scenes': scenes, 'animations': animations }
+    export_user_extensions('gather_gltf_hook', export_settings, plan)
+    active_scene_idx, scenes, animations = plan['active_scene_idx'], plan['scenes'], plan['animations']
+
     if export_settings['gltf_draco_mesh_compression']:
         gltf2_io_draco_compression_extension.compress_scene_primitives(scenes, export_settings)
         exporter.add_draco_extension()

ie. we just give extensions a chance to modify those three variables.

You'd also have to take my suggestion from here about making export_user_extensions more flexible.

@jjcasmar jjcasmar requested a review from emackey February 10, 2020 14:00
@jjcasmar
Copy link
Contributor Author

jjcasmar commented Feb 10, 2020

The code you have posted would work perfectly without modifying the export_user_extensions, no?

Edit: it doesnt work, as we expect the gltf2_object to have an extensions attribute.

So if we allow not to pass a gltf2_object, how can we do the extensions is None check to generate a dict?

Comment on lines 16 to 23
gltf2_object = args[0]
try:
object_extensions = getattr(gltf2_object, "extensions")
except AttributeError:
pass
else:
if object_extensions is None:
setattr(gltf2_object, "extensions", {})
Copy link
Contributor

@scurest scurest Feb 10, 2020

Choose a reason for hiding this comment

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

Easier to do

if args and hasattr(args[0], 'extensions'):
    if args[0].extensions is None:
        args[0].extensions = {}

Like I said before, I think this check should just go away though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you need to do it in all the extensions... By default, the extensions is None, and you dont know which extension is going to be called first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive modify it as you said

@jjcasmar
Copy link
Contributor Author

Any news on this?

@jjcasmar jjcasmar force-pushed the master branch 2 times, most recently from fe76f0e to 083e131 Compare February 14, 2020 19:01
try:
hook(*args, export_settings)
except Exception:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Should the exception get logged to the console? Might be hard to debug without that.

@emackey
Copy link
Member

emackey commented Feb 21, 2020

@jjcasmar Just curious, would this also enable #231, material variants?

@scurest Are your concerns resolved here? Do you think this is a good idea?

@jjcasmar
Copy link
Contributor Author

@emackey It would be possible, but not nice. The problem is that the exporter creates a glTF.Scene, which contains glTF.Nodes. Each node may contain a mesh, which may contain a material. There is no other way to specify a material in the glTF.Scene. So with this patch you would be able to add a dummy node, with a dummy mesh, with a material, but there is no way to add directly a material.

This is a general problem created by how the exporter works. The exporter creates an intermediate representation which is not 1:1 a glTF representation (for example, there is no textures root store).

@jjcasmar jjcasmar force-pushed the master branch 2 times, most recently from 3b53958 to 8ee2eac Compare February 24, 2020 11:26
@julienduroure julienduroure added enhancement New feature or request exporter This involves or affects the export process labels Feb 25, 2020
@julienduroure
Copy link
Collaborator

So ... any problem remaining on this, or is that ready to merge?

@julienduroure julienduroure added this to the Blender 2.83 milestone Mar 9, 2020
@julienduroure
Copy link
Collaborator

@scurest @emackey Any concerns here, or could I merge it?

@scurest
Copy link
Contributor

scurest commented Mar 9, 2020

I don't really have any problems, but I'm not familiar with this code so I'm not really good to ask (my comments were just ways to make stuff shorter).

@jjcasmar
Copy link
Contributor Author

Is there any other review?

@emackey
Copy link
Member

emackey commented Mar 12, 2020

Seems fine. Thanks @jjcasmar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exporter This involves or affects the export process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants