KhronosGroup / glTF-Blender-IO

Blender glTF 2.0 importer and exporter
https://docs.blender.org/manual/en/latest/addons/import_export/scene_gltf2.html
Apache License 2.0
1.5k stars 319 forks source link

Scene extras applying to the current Blender scene on import is unexpected #2395

Open evaera opened 1 week ago

evaera commented 1 week ago

Describe the bug Extras on scenes within a gltf/glb file are written to the current scene when you import a model from them. This is surprising behavior! And the source of a bug we encountered: if the extra is the same name as a property that belongs to a scene (like, in the Properties panel), it changes the property (that's just how Blender properties work).

As part of our pipeline, we have a "file version" extra that's exported on the glTF scene, and it's also present on the blender scene. Importing a glb with the old file version would alter the "file version" property of the scene in the Blend file, which got saved and broke some things. Now that we know this happens, of course we can work around it.

Conceptually, I don't think it makes sense for the importer to work this way. When I import a glb model file, I'm not importing the scenes from the file- they don't turn into Blender scenes. I'm getting the nodes and meshes from within the glb and they're just put into my current scene. I don't want the importer to be "leaky" or have side effects like this; when I delete the thing I imported, I expect its traces to be gone as well.

To Reproduce

  1. Have a glTF file with an extra on the scene
  2. Import it into Blender
  3. The extra is present on the Blender scene

Expected behavior I don't want this to happen, preferably the extras aren't applied to the scene at all.

Screenshots image

.blend file/ .gltf (mandatory) leaky-cube.zip

Version

julienduroure commented 1 week ago

Hello,

Yes, Scene is a special case, because this is the only "object" that already exists in Blender when we importing. We should probably avoid overwrite existing keys of custom properties.

(Not sure if there are case where the user want to overwrite them?)

evaera commented 2 days ago

I'm afraid that simply not overwriting existing properties doesn't quite fix the issue I presented above. Property Definitions, such as the "File Version" property in the original post, do store their values in the dictionary part of the object (the same place regular Custom Properties are stored). However, they actually aren't created until the value is modified. The default value can be read via attribute access (obj.key) but they are not defined yet in the dictionary part (obj["key"]).

So simply checking if the key exists ("key" in obj) isn't enough to determine if it will change an existing property. You could technically check if the Property Definition exists in the object, like "key" in obj.bl_rna.properties.

But I still feel like this is fragile. If the addon that provides those properties simply isn't loaded at the time, there's still a chance of introducing unintended property changes. I think it's safer to just not import extras onto the scene at all (unless the user opts into this).

evaera commented 2 days ago

By the way, I know you’re experienced with Blender and probably are familiar with a lot of what I covered. I just wanted to be thorough, so sorry if it felt a bit over-explained!