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

Better filtering of exported nodes #1126

Merged
merged 2 commits into from Oct 20, 2020
Merged

Better filtering of exported nodes #1126

merged 2 commits into from Oct 20, 2020

Conversation

emackey
Copy link
Member

@emackey emackey commented Jul 16, 2020

This PR deals with parent nodes and ancestor nodes that have been filtered out of glTF export, typically by "use_selected" failing to select the parents of live children. Previously, such nodes would be completely excluded, leaving a hole in the scene graph with a disconnected section below.

In this PR, if a given node fails the export filter, but that node has descendants that pass the export filter, then the failed node is still a critical node to include in the glTF node hierarchy. The failed node will not export any mesh, skin, camera, or weights. But as a parent or ancestor of live descendants, the node will still include its transformations, animations, extras, and extensions, which may have visible effects on the descendants.

This PR could use a bit of additional testing, with animations and skinning in play.

Other exporters don't do this, but eventually I want an option for it.
@scurest
Copy link
Contributor

scurest commented Jul 17, 2020

FWIW I tested this with some armatures and animation (object parented to animated bone, object skinned by arma in different tree) and I couldn't break it, so kudos!

I did find that it overapproximates what to export sometimes. It appears to always export all bones wherever they exist.

out

So there are some cases where master, when it works, is better

out2

I don't know how hard this would be to fix, but obviously those extra empty nodes are invisible in actual viewers, so better to overapproximate than under.

@scurest
Copy link
Contributor

scurest commented Jul 30, 2020

@emackey So is this waiting for something? This fixes #193, #1030, #1066.

@julienduroure
Copy link
Collaborator

We need to check how this work with collection and linked collection

@scurest
Copy link
Contributor

scurest commented Jul 30, 2020

Tested with linked collections. It doesn't work.

By "collections", do you mean if you click on a collection in the outliner like this and export with "Selected Objects" it should export the Cube? That doesn't work either.

col

@scurest
Copy link
Contributor

scurest commented Jul 30, 2020

For linked collections, is it enough to check if the dupli_object_parent is selected?

Patch
diff --git a/addons/io_scene_gltf2/blender/exp/gltf2_blender_gather_nodes.py b/addons/io_scene_gltf2/blender/exp/gltf2_blender_gather_nodes.py
index f05f21e..7a405ae 100644
--- a/addons/io_scene_gltf2/blender/exp/gltf2_blender_gather_nodes.py
+++ b/addons/io_scene_gltf2/blender/exp/gltf2_blender_gather_nodes.py
@@ -60,7 +60,7 @@ def __gather_node(blender_object, library, blender_scene, dupli_object_parent, e
     # If blender_scene is None, we are coming from animation export
     # Check to know if object is exported is already done, so we don't check
     # again if object is instanced in scene : this check was already done when exporting object itself
-    if not __filter_node(blender_object, blender_scene, export_settings):
+    if not __filter_node(blender_object, blender_scene, dupli_object_parent, export_settings):
         if children:
             # This node should be filtered out, but has un-filtered children present.
             # So, export this node, excluding its camera, mesh, skin, and weights.
@@ -113,7 +113,7 @@ def __gather_node(blender_object, library, blender_scene, dupli_object_parent, e
     return node
 
 
-def __filter_node(blender_object, blender_scene, export_settings):
+def __filter_node(blender_object, blender_scene, dupli_object_parent, export_settings):
     if blender_object.users == 0:
         return False
     if blender_scene is not None:
@@ -125,8 +125,11 @@ def __filter_node(blender_object, blender_scene, export_settings):
             else:
                 # Not instanced, not linked -> We don't keep this object
                 return False
-    if export_settings[gltf2_blender_export_keys.SELECTED] and blender_object.select_get() is False:
-        return False
+    if export_settings[gltf2_blender_export_keys.SELECTED]:
+        if dupli_object_parent and bpy.data.objects[dupli_object_parent].select_get():
+            return True
+        if blender_object.select_get() is False:
+            return False
 
     return True
 

edit: No, it's not, since dupli_object_parent is only set for the direct children of the linked collection object, not the grandchild, etc.

@julienduroure julienduroure added enhancement New feature or request exporter This involves or affects the export process Mesh_&_Object labels Aug 10, 2020
@julienduroure
Copy link
Collaborator

Was thinking about another way to solve the issue:

  • Construct a virtual tree as done in importer, calculating world matrix for each node
  • Based on that, using all roots of this virtual tree to recursively go inside tree

@scurest
Copy link
Contributor

scurest commented Sep 16, 2020

I think so too, two passes are necessary.

@emackey
Copy link
Member Author

emackey commented Sep 16, 2020

@julienduroure That sounds good, but would it involve "skipping" intermediate nodes, leaving them out of the tree? Sometimes, multiple children depend on a common parent node that is turned off for export, but the parent may have an animation applied that affects all of the children's locations. Apologies if I misunderstood your suggestion.

@julienduroure
Copy link
Collaborator

Checking animation on node must be part of checks that need to be done in order to know if we can skip a node or node.
I was mostly thinking about upper nodes (from blender root to the node, if root is not selected), but we can extend the solution to intermediate node too. Will try to write some quick drawing to explain

@scurest
Copy link
Contributor

scurest commented Oct 20, 2020

Is this blocked for "the perfect is the enemy of the good" reasons?

@emackey
Copy link
Member Author

emackey commented Oct 20, 2020

@scurest This started to touch too many edge cases and Blender features I'm not well-versed on. We last talked about two-pass processing here with support for linked collections, armatures, etc., which are not tools I normally use in my own simplified workflow. I'm unsure how best to get this moving again. If you have ideas for that, you could make a branch off this branch, or a whole separate implementation if that's easier. Otherwise, I'll try to come back around to this, eventually, but it won't be quick.

@scurest
Copy link
Contributor

scurest commented Oct 20, 2020

What I mean is, why can't it be merged as is? Linked collections don't work in master either.

@emackey
Copy link
Member Author

emackey commented Oct 20, 2020

Ah, if this doesn't make things worse, then yes, let's merge it. Not sure if Julien might be away for a couple days though...

Note that Blender 2.91 "Bcon 3" starts tomorrow, meaning bug fixes only for that particular version. But I do consider this a bug fix, so if it doesn't break new things it can go into 2.91.

@emackey emackey added this to the Blender 2.91 milestone Oct 20, 2020
@julienduroure
Copy link
Collaborator

Hello,
I'm here :)
Will merge it soon

@julienduroure julienduroure merged commit f9e773a into master Oct 20, 2020
2 checks passed
@scurest
Copy link
Contributor

scurest commented Oct 20, 2020

Neat. I can confirm that #193 and #1030 now work with master. Can someone close?

@TheKruspe
Copy link

TheKruspe commented Jan 18, 2021

Is it possible to export an object without its parent nodes?

@emackey
Copy link
Member Author

emackey commented Jan 18, 2021

Without parent nodes, the exported object ends up as an orphan in the scene graph, so does not render at all.

@scurest
Copy link
Contributor

scurest commented Jan 18, 2021

@TheKruspe If you mean like

BLENDER                             GLTF

A         (unselected)     ====>    B
└─ B      (selected)                └─ C
   └─ C   (selected)

Then no, the exporter cannot do that; A always gets pulled along.

@emackey
Copy link
Member Author

emackey commented Jan 19, 2021

Right, and in the case @scurest mentions, "A" might have transformations and/or animations applied to it, that B and C are required to inherit. So you can't just declare B to be a new root of the graph.

@scurest
Copy link
Contributor

scurest commented Jan 19, 2021

I mean, you could. A's transforms would be folded into the transform for B. You can pretty much use whatever hierarchy you want; just compute all local transforms as (parent world).inverted() @ (self world).

@julienduroure
Copy link
Collaborator

This is one of the purpose of #1244
I locally performed some experiments recently on caching/hierarchy. I will write a proposition soon

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 Mesh_&_Object
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants