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

Editor: Selection box should reflect current animation state #18245

Closed
WREQI opened this issue Dec 26, 2019 · 13 comments · Fixed by #26919
Closed

Editor: Selection box should reflect current animation state #18245

WREQI opened this issue Dec 26, 2019 · 13 comments · Fixed by #26919

Comments

@WREQI
Copy link

WREQI commented Dec 26, 2019

image

The parameterized model created by blender, the bounding box calculated by the box after the import is the largest, and does not surround the actual model size
Breaker_NM1630S.glb.zip

@donmccurdy
Copy link
Collaborator

Could you share a model so that we can reproduce this?

You can drag .ZIP files into GitHub comments.

@WREQI WREQI closed this as completed Dec 26, 2019
@WREQI
Copy link
Author

WREQI commented Dec 26, 2019

I uploaded this model, sorry, I accidentally closed the comment, thank you

@WREQI WREQI reopened this Dec 26, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 26, 2019

It seems the bigger AABB is related to: #17940

R111:

image

R110:

image

So in some sense, it's the expected behavior.

@WREQI
Copy link
Author

WREQI commented Dec 26, 2019

image

This is a parametric model. The bounding box obtained after the model deformation is still incorrect.Tested with version 110

@zeux
Copy link
Contributor

zeux commented Dec 26, 2019

Note that in r110, morph targets weren’t included into the bounding box calculation at all in expandByObject, so the result could be too small.

r111 computes the conservative maximum that takes into account all targets with assumed boundary weights of +-1. The result could be too large if the required weight ranges are smaller.

Neither r110 nor r111 has a way to compute a precise bounding box using currently assigned morph weights.

@donmccurdy
Copy link
Collaborator

Our definitions of "incorrect" and "too small" here depend on some assumptions of what the bounding box is being used for... for culling and raycasting, a conservative maximum accounting for skinning and morph targets is a good thing. For UI manipulation in the editor, a snapshot of the objects bounds now is probably what users expect.

I think the changes in #17940 are right, and a conservative geometry.boundingBox is best for internal usage like culling and raycasting. Perhaps this should be considered a feature request for the Editor, to use a tighter bounding box for the UI?

Alternatively, I could imagine having an API that lets users access an approximate bounding box for an object given the current frame without iterating over all vertices for that frame, based on cached min/max values on BufferAttribute properties and some special handling for Skeletons. But that's not the API we have currently, and I don't know how many applications would practically need that.

@zeux
Copy link
Contributor

zeux commented Dec 26, 2019

Just to clarify that #17940 is not at fault; before that change the bounding box was computed irrespective of morph targets,

What is causing this is that in this model, the morph targets are used for major changes to the model's size; the bounding box is computed assuming worst case parameters L & W & H (1), but of course if you set them to their initial values (0), the object is much smaller.

L&W&H = 0:

image

Before #17940, the bounding box was always equal to the extents of this object, even if L/W/H were larger.

L&W&H = 1:

image

After #17940, the bounding box is now always equal to the extents of this object, even if L/W/H are smaller.

So I believe that now the behavior in three.js is actually as intended, and returning the bounding box that can be used for culling. The previous behavior seems strictly worse.

The only way to achieve "intuitive" behavior for editor here is to implement a feature that computes a precise bounding box that takes into account all morph targets (and probably skinning transformation?).

@donmccurdy
Copy link
Collaborator

Not suggesting any problem with #17940, but I think the OP was expecting a bounding box that ignored the presence of morph targets entirely... The three.js Editor doesn't support direct manipulation of morph targets, although you can play/pause animations that could affect them.

@zeux
Copy link
Contributor

zeux commented Dec 27, 2019

I guess, although since GLTF files can specify arbitrary starting values for morph weights, ignoring morph targets seems wrong even for the existing editor.

@donmccurdy
Copy link
Collaborator

The Editor is using BoxHelper, rather than displaying geometry.boundingBox directly. I think it would be appropriate for BoxHelper to use the current frame's bounding box rather than the more conservative box used in the renderer — it already has an .update() method that we expect users to call when the content changes.

@donmccurdy donmccurdy changed the title The bounding box of the parametric deformation model is incorrect EDIT: Selection box should reflect current animation state Dec 27, 2019
@donmccurdy donmccurdy changed the title EDIT: Selection box should reflect current animation state Editor: Selection box should reflect current animation state Dec 27, 2019
@WREQI
Copy link
Author

WREQI commented Dec 27, 2019

I guess, although since GLTF files can specify arbitrary starting values for morph weights, ignoring morph targets seems wrong even for the existing editor.

Not only the bounding box is correct, but also the center coordinates of some objects are incorrectly obtained after deformation.The central bid of the child object obtained by updateMatrixWorld(true),getWorldPosition has not changed

@donmccurdy
Copy link
Collaborator

@WREQI The box you see is accounting for all possible deformations of the morph targets on the mesh. So the box may look much larger than the shape you see on screen, and the center may be off. I've updated the issue's title to clarify this.

@mrdoob mrdoob added this to the r113 milestone Jan 2, 2020
@mrdoob mrdoob modified the milestones: r113, r114 Jan 29, 2020
@mrdoob mrdoob modified the milestones: r114, r115 Feb 29, 2020
@mrdoob mrdoob modified the milestones: r115, r116 Mar 25, 2020
@mrdoob mrdoob modified the milestones: r116, r117 Apr 30, 2020
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob removed this from the r118 milestone Jun 24, 2020
@mrdoob mrdoob added this to the r146 milestone Sep 29, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@mrdoob mrdoob modified the milestones: r147, r148 Nov 30, 2022
@mrdoob mrdoob modified the milestones: r148, r149 Dec 22, 2022
@mrdoob mrdoob modified the milestones: r149, r150 Jan 26, 2023
@mrdoob mrdoob modified the milestones: r150, r151 Feb 23, 2023
@mrdoob mrdoob modified the milestones: r151, r152 Mar 30, 2023
@mrdoob mrdoob modified the milestones: r152, r153 Apr 27, 2023
@Mugen87 Mugen87 modified the milestones: r153, r154 Jun 1, 2023
@mrdoob mrdoob modified the milestones: r154, r155 Jun 29, 2023
@mrdoob mrdoob modified the milestones: r155, r156 Jul 27, 2023
@mrdoob mrdoob modified the milestones: r156, r157 Aug 31, 2023
@mrdoob mrdoob modified the milestones: r157, r158 Sep 28, 2023
@Mugen87 Mugen87 modified the milestone: r158 Oct 7, 2023
@Mugen87 Mugen87 removed this from the r158 milestone Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants