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

Fixed broken gizmo manipulations for Model component #8769

Merged
merged 1 commit into from Apr 9, 2024

Conversation

matgis
Copy link
Contributor

@matgis matgis commented Apr 8, 2024

  • Fixed a regression where the move gizmo would translate rotated Models along the wrong axis.
  • Fixed a regression where the manipulator gizmo would not be properly aligned to Model pivot points.

Fixes #8756

@AGulev
Copy link
Contributor

AGulev commented Apr 8, 2024

Does the fix work for both modes?
CleanShot 2024-04-08 at 18 03 25@2x

(let [renderable (first renderables)
node-id (:node-id renderable)
request-id [::outline node-id]]
(render/render-aabb-outline gl render-args request-id renderables rcount))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly indentation. Wrapped in (when (= pass/outline (:pass render-args)) ...) instead of asserting on the pass. We needed to add the renderable to pass/selection or pass/opaque-selection for it to be considered selectable.

:select-batch-key model-scene-resource-node-id
:passes [pass/outline]}
:select-batch-key :not-rendered ; The render-fn only does anything during the outline pass.
:passes [pass/outline pass/opaque-selection]} ; Include in a selection pass to ensure it can be selected and manipulated.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the correct way to do this is to have the root scene include the :node-id of the ResourceNode (i.e. the ModelSceneNode in our case), and include a :renderable that is assigned to either pass/selection or pass/opaque-selection. It doesn't have to render anything, but this ensures it can be included in the :selected-renderables output of the SceneRenderer.

None of the :children scenes should specify a :node-id, unless they are actually from different nodes in the graph that can be transformed independently using the gizmo.

@matgis matgis requested review from Jhonnyg and vlaaad April 8, 2024 16:15
@matgis matgis self-assigned this Apr 8, 2024
@matgis matgis added bug Something is not working as expected editor Issues related to the Defold editor labels Apr 8, 2024
@matgis
Copy link
Contributor Author

matgis commented Apr 8, 2024

Does the fix work for both modes?

Yep!

Copy link
Contributor

@vlaaad vlaaad left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -355,8 +352,8 @@
:renderable {:render-fn render-outline
:tags #{:model :outline}
:batch-key nil ; Batching is disabled in the editor for simplicity.
:select-batch-key model-scene-resource-node-id
:passes [pass/outline]}
:select-batch-key :not-rendered ; The render-fn only does anything during the outline pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit confusing, at first I thought this is a specially recognized key, but it's not :)

@matgis matgis merged commit e6dcea0 into editor-dev Apr 9, 2024
6 checks passed
@matgis matgis deleted the DEFEDIT-8756-wrong-axis branch April 9, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working as expected editor Issues related to the Defold editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants