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

feat: Renderer hotkeys and select multiple entities #869

Merged
merged 21 commits into from
Jan 19, 2024
Merged

Conversation

cyaiox
Copy link
Contributor

@cyaiox cyaiox commented Jan 15, 2024

This PR allows the users to use the hotkeys in the editor:

  • DEL (backspace / del)
  • COPY (ctrl + c)
  • PASTE (ctrl + v)
  • ZOOM IN = or +
  • ZOOM OUT - or _
  • RESET CAMERA space

It also allows the users to select multiple entities while pressing the key: ctrl and interact with them, updating the transform component.

Closes: decentraland/sdk#1064
Closes: decentraland/sdk#1065

Copy link
Contributor

github-actions bot commented Jan 15, 2024

Test this pull request

  • The @dcl/sdk package can be tested in scenes by running

    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/editor-hotkeys/dcl-sdk-7.3.37-7585786157.commit-580d1f6.tgz"
  • To test with npx init

    export SDK_COMMANDS="https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/editor-hotkeys/dcl-sdk-commands-7.3.37-7585786157.commit-580d1f6.tgz"
    npx $SDK_COMMANDS init
  • The @dcl/inspector package can be tested by visiting this url

    • Or by installing it via NPM
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/editor-hotkeys/@dcl/inspector/dcl-inspector-7.3.37-7585786157.commit-580d1f6.tgz"
  • The /changerealm command to test test in-world

    /changerealm https://sdk-team-cdn.decentraland.org/ipfs/feat/editor-hotkeys-e2e
    
  • You can preview this build entering:
    https://playground.decentraland.org/?sdk-branch=feat/editor-hotkeys

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

Attention: 137 lines in your changes are missing coverage. Please review.

Comparison is base (9508398) 66.67% compared to head (7ff13ff) 66.34%.

Files Patch % Lines
packages/@dcl/inspector/src/hooks/useHotkey.ts 0.00% 62 Missing and 1 partial ⚠️
...r/src/lib/sdk/operations/update-selected-entity.ts 32.50% 27 Missing ⚠️
...ctor/src/lib/babylon/decentraland/gizmo-manager.ts 73.73% 26 Missing ⚠️
...ckages/@dcl/inspector/src/components/Tree/utils.ts 0.00% 5 Missing ⚠️
...l/inspector/src/lib/babylon/decentraland/camera.ts 28.57% 5 Missing ⚠️
...ponents/Warnings/MultipleEntitiesSelected/index.ts 0.00% 1 Missing and 1 partial ⚠️
...ctor/src/components/Warnings/SdkOperation/index.ts 0.00% 1 Missing and 1 partial ⚠️
...cl/inspector/src/hooks/editor/useGizmoAlignment.ts 91.66% 2 Missing ⚠️
packages/@dcl/inspector/src/hooks/sdk/useTree.ts 0.00% 2 Missing ⚠️
packages/@dcl/inspector/src/redux/sdk/index.ts 90.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
- Coverage   66.67%   66.34%   -0.33%     
==========================================
  Files         494      498       +4     
  Lines       14831    15081     +250     
  Branches     1912     1930      +18     
==========================================
+ Hits         9888    10005     +117     
- Misses       4634     4764     +130     
- Partials      309      312       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

cloudflare-workers-and-pages bot commented Jan 15, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7ff13ff
Status: ✅  Deploy successful!
Preview URL: https://7f9fb33e.js-sdk-toolchain.pages.dev
Branch Preview URL: https://feat-editor-hotkeys.js-sdk-toolchain.pages.dev

View logs

@@ -2,7 +2,8 @@
"name": "@dcl/inspector",
"version": "0.1.0",
"dependencies": {
"@dcl/asset-packs": "^1.8.0"
"@dcl/asset-packs": "^1.8.0",
"hotkeys-js": "^3.13.3"
Copy link
Member

Choose a reason for hiding this comment

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

oops, this should be a devDependency

@cazala
Copy link
Member

cazala commented Jan 16, 2024

I notice a few things are not working the way I would expect them.

On one side, the rotation of a multiselection is applied on each entity, instead of rotating the other entities around the one with the gizmo:

rotation-multiselect.mp4

Also when selecting entities that are nested, the translation is applied on each entity and results in weird behaviors like this:

nesting-multiselect.mp4

I think most intuitive way to treat multiselection is to temporary nest the other selected entities under the originally selected one, and then move them back to their previous position in the tree. When reparenting entities we already recompute the matrices so they stay in the same global position/rotation/scale. This would solve the rotation issue and the nested issues from above, it would also make it work in realtime, as it currently works when you nest some entities under a parent.

Something like this:

  1. Select entity A
  2. Press and hold CTRL key
  3. Select entities B and C, they are nested under A (remember their original positions in the tree for later), their matrices will be updated but they will stay in the same global position
  4. Transform A (translate, rotate, scale, it would apply the transformation according to the nested entities as it currently does, moving/rotating/scaling around the parent)
  5. Release CTRL key
  6. Entities B and C and returned to their original positions in the entity tree, their matrices will be updated so they stay in the same place globally but with their new parents.

@cazala
Copy link
Member

cazala commented Jan 16, 2024

Another thing that I think would be necessary for the multiselect is showing the other selected entities in the Hierarchy/Tree in the left sidebar. Also I think allowing multiselecting from there should be great to have, like just pressing CTRL and selecting from the tree instead of the editor.

@cazala
Copy link
Member

cazala commented Jan 19, 2024

This is looking great! I found one issue with the undo though, when I undo some changes I did with multiselect, it only applies the undo to the originally selected entity:

undo-multiselect.mp4

Another minor thing, I think the warning about the ancestor is working correctly (it does not let me select a parent entity of the originally selected one) but the copy seems wrong to me.

In this example I have a tree with entities "Ancestor" and "Child", which is nested to "Ancestor". When I start the multiselection from "Child" and I try to add later "Ancestor", the warning reads "An ancestor of this entity has already been selected". The "this entity" I understand it as the one I just clicked and made the warning appear, which is "Ancestor". So the entity that is "already selected" is not "An ancestor of this entity" but "A child of this entity".

ancestor-warning.mp4

Maybe a better wording would be "You can't select an ancestor of an already selected entity" ?

@cyaiox cyaiox merged commit 58deb0d into main Jan 19, 2024
8 of 10 checks passed
@cyaiox cyaiox deleted the feat/editor-hotkeys branch January 19, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select and Transform multiple entities Add hotkeys to the inspector editor
2 participants