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

UI Handle overload with scaling #546

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

austinbhale
Copy link
Contributor

Hey Nick, I wrote a handle extension for scaling but noticed that you had previous and active pinch points in the source code, so I thought it could fit nicely here instead. I added the scaling to the clipboard for the DemoUI script for testing it out. Let me know what you think!

@austinbhale austinbhale changed the title Develop UI Handle override with scaling Dec 12, 2022
@austinbhale austinbhale changed the title UI Handle override with scaling UI Handle overload with scaling Dec 12, 2022
@maluoi
Copy link
Collaborator

maluoi commented Dec 12, 2022

Very nice! I had been thinking about this with a new Transform object instead of a separate scale, but I guess I may have been overthinking things, this looks plenty good enough from a public API perspective!

This is missing two things I'd consider to be important:

  • This should also include the scale on the Hierarchy stack, so child items scale with the Handle! Handle adds to Hierarchy via ui_push_surface, which will need to take a scale argument as well for this to work.
  • Scale should be 1:1 with hand movement, your hand should appear to be at the same point on the model as it scales up! Currently it feels like the scale is a multiple of hand movement in this example.

refactor: bit flags for different movement types
fix: ui push surface includes an optional scale vector
@austinbhale
Copy link
Contributor Author

Thanks for taking a look! Thought I'd give an update on the latest changes before going any further:

  • movement could be set via a bit flag, this simplifies the handle logic and gives some more flexibility to devs
  • push surface with a scale arg is done in native code, not exposed to the API?
  • changed the float arg to a vector argument in case we look at nonuniform scaling and to make it less hassle to create a scale vector
  • the uniform scaling includes a starting distance and scale to continuously scale the object as long as two hands are manipulating it

I believe I addressed your first point, but I'm a bit unclear on the second point. I checked MRTK in Unreal and Unity and they perform this type of uniform scaling. However, 1:1 scaling sounds like it'd be tricky due to the starting pinch points having to relate to the bounding box. I'll take a crack at it, but I think it's nice to have the uniform scaling as an option as well.

@maluoi
Copy link
Collaborator

maluoi commented Dec 13, 2022

Definitely a good improvement! I believe you did actually get the 1:1 scaling mostly correct on this one, but in the UI scene it's hidden underneath a double scale. You're providing the scale to clipboard.Draw where it's already inherited from HandleBegin.

After that it still feels a little off. I believe this is because of where the scale is happening from, it's currently scaling relative to the Handle position, where it should be scaling relative to the center of the 2 handed grab! This means that the position will also need to 'scale' towards/away from your hands as part of the scaling math. Might be a little tricky to communicate that via text, if you have no idea what I'm talking about, lemme know and I'll try recording a snippet!

I'm skeptical on Vec3 for scale, and I think that bit probably should be rolled back. ui_push_surface should 100% remain as a uniform scale, just the float, I can't really think of any good use for non-uniform scale on UI (though I'd be open to argument). On UI.Handle, it's a bit more debatable, but mostly because that function probably belongs in some future Interaction class instead of UI. The big obstacle for UI.Handle though is that a proper non-uniform scale probably needs a whole interface to work, like a transform cage/box, I don't think that can be done by hand gestures alone. I'd rather have something like a UI.ManipulationBoxBegin or something later, and that could have Vec3 for scale instead of Handle!

The bit-flag thing crossed my mind when I first looked at it, but I feel like it was actually pretty nicely handled by just providing or not providing the scale value? Do you think the bit flag is necessary here, or improves the situation? Would love to hear an example where this is better than just omitting or providing the scale var :)

Thanks for your work here, pretty exciting!

Austin Hale added 2 commits December 13, 2022 12:09
fix: roll back to float for scaling args
feat: demo bit flag transforms for damaged helmet
@austinbhale
Copy link
Contributor Author

Just pushed most of your helpful feedback, thanks!

Whoops there was a double scale in that demo, should be accounted for now. The UI demo turned into a placeholder for showing the different move types :)

For scaling from the center of the 2 handed grab, is that scaling from the midpoint between the two starting pinch points? And the location is offset from there to be relative to that midpoint? If you could make a quick video, or pseudo code that'd be very helpful!

I agree that a float would be better passed for scaling the UI surfaces. For the handle, allowing a Vec3 to be passed in for non uniform scaling might make the handle method take on too many tasks at once, so a different manipulation box component could be clearer.

Now on the demo for the bit-flag change! Doing a pure rotation movement is a bit strange honestly, but I like the fact that I can scale without moving the model's position. Or completely forgo the rotation in some cases. Along with providing more transform capabilities, I found a neat trick. If you pass in the float to scale the UI surface, we can use that same scaled hierarchy without having to scale the object! It's a convenient way to scale a model as well as keep the same scaled hierarchy no matter the switching between move types.

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.

None yet

2 participants