Import updates

[keianhzo]

• Add support for newer import API and refactor some code: https://github.com/KhronosGroup/glTF-Blender-IO/tree/master/example-addons/example_gltf_importer_extension
• Fix media-frame yup issues and expose a global import property for yup

[Exairnous]

Thank you for the PR, I'll review this soon.

[Exairnous]

Sorry for the wait on this review, the loop-animation default name stuff took longer than I expected, plus there was a lot to dig into here.

I think this is generally headed in the right direction. I like how you've consolidated/clarified some things and made things consistent with the exporter, so this is good. Some general comments (I've left more specific in-code comments as well):

• Nit: I know the importer example uses import_settings, and consistency is good, but perhaps in this case it would be a good idea to use gltf and break with that example. As far as I can tell, what is passed as import_settings represents the entire gltf file data structure and is referred to as gltf internally, plus gltf seems far more descriptive to me than import_settings e.g. gltf.data.nodes makes a lot more sense to me than import_settings.data.nodes and gltf.import_settings vs import_settings.import_settings. What do you think?

• Nit: I see you've removed all my debug prints. While they were getting a bit much and could use some paring down, I'd prefer to leave prints for the component name + component property names and values with a blank line after each component block, so that it's easy to verify that the components have been imported correctly. When we're ready to merge this into master we can remove them then.

• I don't like mixing in the paired down Spoke imports as separate components with the rest of the component definitions. I'm not exactly sure what the best way to handle it is, but the two ideas I like best so far are:

  1. Having the Spoke component name map to the relevant add-on component and adding in a separate gather_import_componet_name class method that handles the import when the Spoke component is imported. E.g. 'background' gets defined as an additional reference to environment-settings and so when 'background' gets passed to get_component_by_name it returns the environment settings class and then the HubsComponent base class can be set up to auto-magically call the gather_import_background method instead of the standard gather_import method. (I can upload a branch with an example implementation if you want)

  Or

  2. Moving the Spoke components to their own separate folder within the definitions folder and possibly basing them on a different base class for further differentiation.

• Several Spoke components either aren't handled properly or aren't handled at all (but were previously):

  • The box collider component isn't handled (I believe this maps to the ammo shape).
  • The spawn point component isn't handled (maps to a waypoint with the can be spawn point property enabled).
  • The background component isn't handled correctly (I've left in-code comments with fixes).
  • The spawner isn't handled correctly (it needs my custom handler ported over to a custom gather_import)

• Lightmaps need to use the lightmap texture (currently you're setting them to the base color texture) and they need to use the lightmap UVs (the second set). It would also be nice to detect whether the lightmap is an HDR and set the tone mapping appropriately (although HDR files are recommended, other formats also work to some degree (at least the last time I checked they did) and are much smaller, so they may be used as well). Also, the add_lightmap function in gltf_importer.py should be with the rest of the lightmap stuff in nodes/lightmap.py.

• Your add bone components code doesn't support duplicate bone names. Bone names are only guaranteed to be unique within their armature (two armatures can have bones with the same names, etc.), glTF handles this by index, so to support this you need to get the bones by the armature children indexes. I've left some in-code comments with code that should support this. (I know that Hubs itself doesn't support duplicate bone names properly, but I hope this limitation will eventually be fixed)

• If the armature and at least one bone isn't weighted to a mesh, the glTF importer will import them as empties, rather than armatures/bones. I know this is on the glTF side, but we should probably try to handle this somehow, either by detecting it and converting the empties to proper armatures/bones or by applying the components to the empties. Another problem is that the sizing doesn't seem to be recreated properly, but I'm not sure if there's anything we can do about that. This doesn't need to be handled in this PR, but it's something we need to address before we can offer full bone support.

• Also a very minor nit: it seems like your comment about bones being created after armatures is repeated more often than necessary.

• Blender versions older than 3.1 don't display the import settings panel in the import dialog. This would be nice to support, perhaps it can manually be appended to the already present panel for the older versions?

On a related topic, when we add support for reflection probes we'll need to replace the empties with Blender light probe objects and then add the components to that, so perhaps this can be handled generically for bones as well.

Edit: Another thing I'm wondering about for bone support is if we can/should override glTF's bone direction property and always use the Blender setting?

[Exairnous]

Nit: I'd prefer it if this was passed the gltf representation itself, rather than just the vnodes because it's simpler and allows access to a much wider range of data if needed (as this function develops to handle more situations I think we may need the extra data e.g. image textures are a likely candidate for needing this)

[keianhzo]

Thanks for the review and the suggestions, I've updated a few things. Overall this PR just wanted to set the direction for the importer so I'd prefer merging it and move all this conversations to the official repo PR rather than continue the development here so I've updated a few things but I'll wait until we merge this to continue the conversation. So if you are ok with it we can merge this and keep on iterating on the main PR.

A few comments:

Nit: I know the importer example uses import_settings, and consistency is good, but perhaps in this case it would be a good idea to use gltf and break with that example.

I was mostly following the naming pattern that we had in the exporter but I don't have a string opinion. Feel free to update in your PR when we merge this branch.

Nit: I see you've removed all my debug prints.

I rather adding the debugs locally while developing than committing them as part of the work to avoid leaving unwanted debug logs unintentionally.

I don't like mixing in the paired down Spoke imports as separate components with the rest of the component definitions.

I haven't really paid much attention to the Spoke import process yet. I think it would be great to fully support it so this makes sense. We can continue discussing in the main PR when this is merged.

Lightmaps need to use the lightmap texture (currently you're setting them to the base color texture) and they need to use the lightmap UVs (the second set)

I haven't seen issues with this.

[Exairnous]

You're welcome. Yeah, it would be nice to have stuff linked up to the discord channel again, and most of the stuff can be handled outside this PR. There are a couple things though that should probably be addressed before merging, although I can handle them after, if you want.

Nit: I know the importer example uses import_settings, and consistency is good, but perhaps in this case it would be a good idea to use gltf and break with that example.

I was mostly following the naming pattern that we had in the exporter but I don't have a string opinion. Feel free to update in your PR when we merge this branch.

Ah, that makes sense. From what I can tell, export_settings is a dictionary of export settings that is passed to the user extensions being exported and is defined here:
https://github.com/KhronosGroup/glTF-Blender-IO/blob/638f0ac9bdb5bdbdf11f8249f0843a7c78027db2/addons/io_scene_gltf2/__init__.py#L543

while import_settings starts off originally as gltf, but then gets changed to gltf_importer and then is recommended by the example to be received as import_settings in the user extensions, see:
https://github.com/KhronosGroup/glTF-Blender-IO/blob/d5b6052506445ab092dce3708175fcd8902d4073/addons/io_scene_gltf2/blender/imp/gltf2_blender_node.py#L42
and
https://github.com/KhronosGroup/glTF-Blender-IO/blob/master/addons/io_scene_gltf2/io/imp/gltf2_io_user_extensions.py

Since this is the case, I will likely change it after this PR is merged and probably open up an issue or PR on the glTF add-on to see about changing it in their stuff as well.

Nit: I see you've removed all my debug prints.

I rather adding the debugs locally while developing than committing them as part of the work to avoid leaving unwanted debug logs unintentionally.

Well, at this stage it's quite useful to have debug prints showing the raw component data from the glTF file and it would be nice not to have to maintain this locally, but I suppose I can.

I don't like mixing in the paired down Spoke imports as separate components with the rest of the component definitions.

I haven't really paid much attention to the Spoke import process yet. I think it would be great to fully support it so this makes sense. We can continue discussing in the main PR when this is merged.

Yeah, this will need a bit of design work, so I'm happy to continue discussing this after merging the PR. I think that GLBs from Spoke will be one of the main use cases right now since GLB scenes aren't yet remixable.

Aside from the Spoke components, the spawner still needs a custom gather_import as it currently causes a python error (apologies for not making the issue clearer). I'm okay with not transferring support for some of the Spoke only components, as this still needs design work, but I would prefer not to merge stuff with known python errors.

Edit: Well, known python errors that are the result of a regression. I realize that with the try/except wrapping that it isn't technically a python error anymore, but it's also an easy fix (you should just need to use the old handle_spawner function as the gather_import), so I think it would be nice to include, but as with the rest of them I'll leave it up to you.

Lightmaps need to use the lightmap texture (currently you're setting them to the base color texture) and they need to use the lightmap UVs (the second set)

I haven't seen issues with this.

I found this interesting, since I had, so I did some digging and realized that you're using the lightmap source index to attempt to get the image directly rather than going through the texture. The index stored for the lightmap is the texture index, and the texture has it's own index for the image and so for some scenes they happen to be the same, but in others they aren't and this results in the discrepancy. In addition to this, the lightmap still needs to use the second set of UVs and non-hdr lightmaps should be handled. I've left code to handle all of this if you want to include it in this PR, or I can commit a fix after it's merged, whatever you like.

So, let me know whether you want to address the spawner, lightmaps, and traceback printing, and then afterwards I'll merge your PR.

[keianhzo]

I've fixed the lightmap import issue, the spoke spawner python error and reverted to printing the stacktrace for importer exceptions. I think we can merge this and follow-up in the main repo and address the remaining things.

[Exairnous]

Yup, will merge.