Hubs-Foundation / hubs

Duck-themed multi-user virtual spaces in WebVR. Built with A-Frame.
https://hubsfoundation.org
Mozilla Public License 2.0
2.13k stars 1.41k forks source link

Fixes #6496 - scale & rotate buttons #6502

Closed DougReeder closed 1 week ago

DougReeder commented 1 month ago

Bisecting showed the PR that introduced the bug was https://github.com/Hubs-Foundation/hubs/pull/6487 Among many changes there, findAncestorWithComponents() needed be called with Rigidbody. Holdable was retained and Deletable and MediaLoader were left out.

If it was a simple oversight, this PR should be a complete fix. If there were reasons to drop Deletable and MediaLoader, we'll need to do more work.

Exairnous commented 1 month ago

@DougReeder I think this looks good. Thanks. @keianhzo can you confirm?

DougReeder commented 1 month ago

I removed Deletable from this code, and you could scale but not rotate objects. :-S

If restoring Deletable and MediaLoader here breaks the Blender grabbables component, it looks like we should revert https://github.com/Hubs-Foundation/hubs/pull/6487

Exairnous commented 2 weeks ago

I did a little digging and it looks like the reason this PR appears to be working is because it's not finding any entity with all of those components, so it's falling back to using the hovered entity. For some reason, the rotate and scale buttons seem to behave differently from the rest of the menu, probably because they are interactible/grabable instead of just single click buttons. So, I think the correct solution, fingers crossed, should be to check if it's a HoldableButton first, and then use that if it is, or if it isn't then check for the Holdable Rigidbody. Here's is some code that I think should work, but I've only done a cursory test of it.

const holdablebutton = findAncestorWithComponent(world, HoldableButton, hovered);
const interactable = holdablebutton ? holdablebutton : findAncestorWithComponents(world, [Holdable, Rigidbody], hovered);
target = interactable ? interactable : hovered

(note, this was manually retyped from a different computer I was testing on, so beware of typos)

@keianhzo What are your thoughts on this reasoning/solution?

DougReeder commented 1 week ago

Withdrawn in favor of PR #6515