cruise-automation / webviz

web-based visualization libraries
https://webviz.io/
Apache License 2.0
2.08k stars 412 forks source link

feature request: `cameraMatrices` prop in `<Worldview />` component #270

Open colinterface opened 5 years ago

colinterface commented 5 years ago

Awesome library, thanks for all you do! 😺

I'd like to supply my own camera matrices to <Worldview /> instead of the cameraState object.

the idea

<Worldview 
  cameraMatrices={{ 
    view, 
    projection
  }}
>

data model

// @flow

type CameraMatrices = {|
  view: Mat4,
  projection: Mat4,
|}

new behavior

<Worldview /> component checks for valid cameraMatrices prop on mount and update, sets them in cameraStore which uses them directly with priority over cameraState

benefits

adds support for arbitrary camera parameters in a standard format, expanding use cases

drawbacks

introduces more complexity to the camera state api

what do you think about this design? i'm eager to contribute if you think it fits.

jtbandes commented 5 years ago

This definitely seems like a good feature to add. Let's think about the API a bit more — some of the properties in cameraState would be replaced by the view matrix and some by the projection matrix, right?

I was thinking it might be nice to group these properties in with the cameraState, but now I realize if both matrices are specified then there's nothing left in cameraState (except that keyboard/mouse controls depend on the values in cameraState, but you said you'd probably want to disable those). I'd be a bit hesitant to keep adding new props to Worldview since it already has so many, but maybe we can change the type of cameraState to include both alternatives CameraState | CameraMatrices? Then it's also more clear that you can't specify both the matrices and the other cameraState properties.

Do you think you (or other users) would want to supply one of these matrices but not the other?

As an point of reference, the XRView interface defined in the WebXR spec provides a projectionMatrix and a transform (which has a position+orientation, and also a combined matrix property).

cc @brianc @vidaaudrey who worked with raw view/projection matrices for a Worldview VR project :)

colinterface commented 5 years ago

Thanks for the feedback! 😸

Just spent some time poking at it this morning and I think I have a plan for moving forward.

I'd be a bit hesitant to keep adding new props to Worldview since it already has so many

I totally see your concern. I think it's a matter of documentation though. There are 3 groups of users who all want a simple explanation (and ideally an example) of the props they need to use.

  1. Someone who just wants to use worldview's internal camera state. They'll want to know that this is the default option, that they can use the default mouse and keyboard controls, how to customize them if they want to, and how they can provide an initial camera state if they need to.
  2. Someone who wants control of the camera state, but doesn't want to deal with matrices. They'll want to know about the cameraState object and how each parameter works. They'll also want to know that they need to use the onCameraStateChange callback to update their camera state so they can use the built in camera controls.
  3. Someone who wants to use their own view and projection matrices. I believe this person will accept that they'll have to provide their own camera controls, though they might be interested in getting the input events and mapping them the same way they would if using the built in camera controls…

Personally I don't need Worldview to manage input events… that's firmly outside of the scope of what I want it to do for me. But it's definitely convenient if we want this to be a one stop shop for building a 3D application. That's a whole other discussion though.

but maybe we can change the type of cameraState to include both alternatives

Turning cameraState into a union type is definitely one option, but I think with the right docs and warnings, having a separate prop might be easier to understand… implementation-wise it's not a huge difference. My gut reaction is generally to avoid union types where possible.

Do you think you (or other users) would want to supply one of these matrices but not the other?

I think in most cases if someone is using matrices, they'll probably provide both. CameraStore provides valid defaults for any values missing in the cameraState object. We could do a similar thing for cameraMatrices, along with a console warning.

As a point of reference, the XRView interface defined in the WebXR spec provides a projectionMatrix and a transform

Using the name transform instead of view makes sense in the context of the XRView interface because it's a transform for the view. But I think this shape works well for our context:

const cameraMatrices = { 
  view, 
  projection 
};

Dealing with view and projection matrices is common in regl and the greater WebGL ecosystem. People using this advanced feature will know that they map to the projection * view part of their vertex shader.

Anyway, it seems like we'll be able to reach alignment one way or another and at this point I'll just need to make a branch for more feedback. Here's my current plan:

  1. Make a story on the cameraState module that allows passing matrices directly
  2. Update the cameraState documentation to clearly lay out the 3 scenarios described above
  3. Update the Worldview component, the CameraStore class, and the camera selectors to support the new behavior.
  4. Open a PR for further discussion.

We have a sprint coming up where we're focusing on FE infra. I'll plan to tackle this during the week of Dec 9. Open to discussing more here but probably won't be able to do much until then.

Does that sound like a reasonable plan?

jtbandes commented 5 years ago

with the right docs and warnings, having a separate prop might be easier to understand

yeah, agreed if we have both docs and warnings, this should be fine.

  1. Make a story on the cameraState module that allows passing matrices directly

we'll have to see about this organization, since if the matrices are a separate prop from cameraState it doesn't make as much sense to group them with the cameraState stories...but they are camera-related, so maybe it's fine. You might find some opportunities to clean up the organization while you're there.

Looking forward to your branch!

colinterface commented 4 years ago

Hey there, we haven't forgotten about this. We have a fork over at https://github.com/hoverinc/webviz/

As we continue to integrate Worldview into our apps, we are finding more tweaks we need to make.

I think our plan to merge might have been a bit premature. It's still something we would like to do, but it might make sense to wait until our changes have stabilized a bit more.

janpaul123 commented 4 years ago

Cool, feel free to make a PR if you think it's stable enough now. 😄