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

Implemented GizmoHelper, ViewportGizmo and ViewcubeGizmo #303

Merged
merged 12 commits into from
Feb 27, 2021

Conversation

w3dot0
Copy link
Contributor

@w3dot0 w3dot0 commented Feb 12, 2021

The camera animation logic could use some help. Could this use react-spring?

Integrating it with OrbitControls and other camera controls is rather hard. Using onUpdate and onTarget to return the control target is hacky, but I don't see a way to get a reference to the controls (and to handle the different types). Any help is appreciated!

  • Documentation updated
  • Storybook entry added
  • Ready to be merged

image

image

@vercel
Copy link

vercel bot commented Feb 12, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/drei/fdx9403xz
✅ Preview: https://drei-git-fork-w3dot0-blendernavigationgizmo.pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 12, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fbebfda:

Sandbox Source
fast-sound-hc4i7 Configuration
drei reflector Configuration
arc-x-pmndrs-colors Configuration

@w3dot0
Copy link
Contributor Author

w3dot0 commented Feb 12, 2021

One other issue - I'm not sure how to position the Gizmo within the Canvas. It currently appears at the top right, but I'm unclear why.

@joshuaellis joshuaellis marked this pull request as draft February 12, 2021 16:49
@drcmda
Copy link
Member

drcmda commented Feb 12, 2021

this would be great to have. as for position and heads-up-display, you can take a look at this: https://codesandbox.io/s/react-three-fiber-viewcube-forked-pgm9m

def dont use react spring for this, if you want to animate, better lerp it.

controls is going to be tough. with drei we could add something to our controls so that they allow movement readout. you could add it to this pr - it would also be useful in many other situations. i just dont know where you'd store that. but if this gizmo had at least the control reference, it could shut it off while moving.

the best way i can think of is forking orbit/trackball. we are suffering from these for a long time and threejs is not going to fix them. they have longstanding bugs and are sealed. if we could say "controls, go there" by code, "zoom here", etc, that would make lots of things easier.

@joshuaellis
Copy link
Member

Thanks for the work @w3dot0! I think there’s some places we can scrub up, but probably best to wait for that until you’ve looked into @drcmda’s suggestions.

@w3dot0
Copy link
Contributor Author

w3dot0 commented Feb 13, 2021

Made a few changes per @drcmda's recommendations (position is now configurable) - horizontal animation seems ok, but vertical seems janky (maybe I need to set the camera up vector during animation) and the distance calculation doesn't seem quite correct either. Demo at https://codesandbox.io/s/bold-browser-i2qr1?file=/src/index.js

I would love to collaborate on "CAD controls" (with zoom to mouse functionality and the common presets/animations baked in.)

@gsimone
Copy link
Member

gsimone commented Feb 13, 2021

Would it be crazy to decouple functionality from presentation?
Something like

<NavigationGizmo>
 {(...whateverIsNecessary) => (<CustomGizmoComponent />)}
</NavigationGizmo>

that would allow users to drop in their own Gizmo renderer.

This can be done at a later time and is not blocking for the PR, of course

@gsimone
Copy link
Member

gsimone commented Feb 13, 2021

I would love to collaborate on "CAD controls" (with zoom to mouse functionality and the common presets/animations baked in.)

This could be an interesting starting point for react-based controls:

  • react-use-gesture to make sense of events
  • animatable with spring out of the box
  • interruptible via props change
  • ergonomic callbacks for react apps

Of course the alternative is just making them for vanilla and using them as usual, but we've been thinking about trying the react way, for a while 🤔

@w3dot0 w3dot0 changed the title Implemented NavigationGizmo inspired by Blender and threejs.org/editor Implemented GizmoHelper, BlenderNavGizmo and ViewCubeGizmo Feb 16, 2021
@w3dot0
Copy link
Contributor Author

w3dot0 commented Feb 16, 2021

Would it be crazy to decouple functionality from presentation?
Something like

<NavigationGizmo>
 {(...whateverIsNecessary) => (<CustomGizmoComponent />)}
</NavigationGizmo>

that would allow users to drop in their own Gizmo renderer.

This can be done at a later time and is not blocking for the PR, of course

Great suggestion! I split up the code into GizmoHelper (functionality) and two presentation components (BlenderViewportGizmo and a basic ViewCubeGizmo).

@w3dot0 w3dot0 changed the title Implemented GizmoHelper, BlenderNavGizmo and ViewCubeGizmo Implemented GizmoHelper, BlenderViewportGizmo and ViewCubeGizmo Feb 16, 2021
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

I've left some thoughts, some of them apply to multiple components, but I didn't want to spam and re-write it x times. If you have any questions or want to discuss anything let me know.

src/core/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/core/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/core/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/core/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/core/GizmoHelper.tsx Outdated Show resolved Hide resolved
src/core/GizmoHelper.tsx Outdated Show resolved Hide resolved
src/core/GizmoHelper.tsx Outdated Show resolved Hide resolved
src/core/GizmoHelper.tsx Outdated Show resolved Hide resolved
src/core/BlenderViewportGizmo.tsx Outdated Show resolved Hide resolved
src/core/BlenderViewportGizmo.tsx Outdated Show resolved Hide resolved
@w3dot0 w3dot0 marked this pull request as ready for review February 16, 2021 16:08
@w3dot0 w3dot0 marked this pull request as draft February 16, 2021 18:18
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Overall it's shaping up really nicely, there's a lot of code to go through! But i think there's a lots to discuss // do after this round to shape it up further.

.storybook/stories/BlenderViewportGizmo.stories.tsx Outdated Show resolved Hide resolved
.storybook/stories/BlenderViewportGizmo.stories.tsx Outdated Show resolved Hide resolved
.storybook/stories/BlenderViewportGizmo.stories.tsx Outdated Show resolved Hide resolved
.storybook/stories/ViewCubeGizmo.stories.tsx Outdated Show resolved Hide resolved
src/core/GizmoHelper.tsx Outdated Show resolved Hide resolved
src/web/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/web/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/web/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
src/web/index.ts Outdated Show resolved Hide resolved
src/web/ViewCubeGizmo.tsx Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

I'm happy with this. @gsimone, @drcmda do you have anything you wanna add? If not I can go ahead and merge it.

@w3dot0 w3dot0 changed the title Implemented GizmoHelper, BlenderViewportGizmo and ViewCubeGizmo Implemented GizmoHelper, ViewportGizmo and ViewCubeGizmo Feb 18, 2021
@w3dot0 w3dot0 changed the title Implemented GizmoHelper, ViewportGizmo and ViewCubeGizmo Implemented GizmoHelper, ViewportGizmo and ViewcubeGizmo Feb 18, 2021
@w3dot0
Copy link
Contributor Author

w3dot0 commented Feb 23, 2021

I've been thinking more about the use case where some, but not all of the Gizmo's controls reflect the orientation of the main scene, like the rotation buttons in Autodesk's ViewCube (https://youtu.be/UlUssM353Rw?t=116).

Would this require adding another (static) camera to the GizmoHelper to use with the "fixed position" Children, or can this be done within the Gizmo component, maybe using the Billboard component?

@joshuaellis
Copy link
Member

Would this require adding another (static) camera to the GizmoHelper to use with the "fixed position" Children, or can this be done within the Gizmo component, maybe using the Billboard component?

Why would this need another camera? Maybe I'm not understanding correctly 🤔 If you think it's worth it, maybe an example impl could be done in a sandbox so we don't mess with this PR before discussion?

@joshuaellis
Copy link
Member

@w3dot0 did you want to change something or could this come later?

@w3dot0
Copy link
Contributor Author

w3dot0 commented Feb 24, 2021

@joshuaellis sorry, been swamped and haven't gotten to creating the example.

I think a combination of Billboard and Html components should work for what I was thinking of (no extra camera required), so I'd say let's merge this if @gsimone and @drcmda are ok with the current structure.

@joshuaellis
Copy link
Member

No worries! Yep, I'll merge tomorrow at some point unless they have objections. Thanks again, look forward to pt. 2

@joshuaellis joshuaellis merged commit 54b2953 into pmndrs:master Feb 27, 2021
@github-actions
Copy link

🎉 This PR is included in version 3.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@drcmda
Copy link
Member

drcmda commented Jun 30, 2021

@w3dot0 does it work with trackballcontrols? i am trying to combine them but it seems to break. could we add this?

repro: https://codesandbox.io/s/trackball-gizmo-n75mo

@w3dot0
Copy link
Contributor Author

w3dot0 commented Jul 1, 2021

@w3dot0 does it work with trackballcontrols? i am trying to combine them but it seems to break. could we add this?

repro: https://codesandbox.io/s/trackball-gizmo-n75mo

@drcmda Thanks for the repro link. It looks like the camera up vector needs to be animated as well, but maybe there are other issues. I can take a look this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants