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

Mismatch between reported viewport from three.js and R3f #1892

Open
looeee opened this issue Dec 6, 2021 · 14 comments
Open

Mismatch between reported viewport from three.js and R3f #1892

looeee opened this issue Dec 6, 2021 · 14 comments
Labels
core target agnostic (normally to do with reconciler) documentation to do with docs

Comments

@looeee
Copy link

looeee commented Dec 6, 2021

I've set up a simple sandbox here - when you click it will toggle the active viewport between two states:

  1. Initial: the full canvas, viewport = [0, 0, canvas width, canvas height]
  2. Toggled: the bottom left quarter of the screen, viewport = [0, 0, half canvas width, half canvas height]

After toggling, the current viewport size reported by three.js and R3F are logged to the console:

  1. Initial (for a canvas with width = 2228, height = 721)
R3f Viewport:  {width: 23.71, height: 7.67, factor: 93.96, distance: 5.0, aspect: 3.09}
Three.js Viewport:  (4) [0, 0, 2228, 721]

  1. Toggled (same canvas)
R3f Viewport:  {width: 23.71, height: 7.67, factor: 93.96, distance: 5.0, aspect: 3.09}
Three.js Viewport:  (4) [0, 0, 1114, 360.5]

As you can see, three.js updates to show the new size. R3F continues to report the viewport as the entire canvas.

I think the issue here is actually semantics - in R3F, the word "viewport" is being used to report various stats about the state of the canvas element combined with the dpr. However, three.js is reporting the value of glViewport.

Since viewport usually has a specific meaning in 3d computer graphics - it's the size of the viewing rectangle into which you will output the scene, in pixels - I'm inclined to say the three.js usage of the term is more correct here.

@drcmda
Copy link
Member

drcmda commented Dec 11, 2021

viewport is in three units, that means width/height would fill the screen. the other values are in pixel units, that's "size" in useThree. when you mutate something in threejs, it's not reactive, there is no way to respond. useThree-size/viewport react to a resize observer or state changes.

when you want to set the viewport that is possible as well but it would be your responsibility to inform the state model. there is a setSize function for that: https://github.com/pmndrs/react-three-fiber/blob/master/packages/fiber/src/core/store.ts#L91 you would use that for the render function as well

const setSize = useThree(state => state.setSize)
  ...
  setSize(width, height)

@CodyJasonBennett CodyJasonBennett added the core target agnostic (normally to do with reconciler) label Dec 11, 2021
@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Dec 11, 2021

Is the expectation for state.viewport to be reactive? Otherwise, this looks correct unless you want width and height to be scaled with factor -- the semantics shouldn't change though.

@looeee
Copy link
Author

looeee commented Dec 12, 2021

Is the expectation for state.viewport to be reactive?

I'm not sure about that. However, I do think that a method such as .getCurrentViewport should report the live state and should ideally report the same value as WebGLRenderer.getViewport. Otherwise it's just confusing.

I also want to highlight that R3F is using the word viewport to mean something non-standard and different from how three.js/WebGL (and AFAIK pretty much all OpenGL/DirectX graphics libraries) use the term.

Here's the normal meaning (it's on the MS DirectX docs but it's the same meaning for OpenGL):

Conceptually, a viewport is a two-dimensional (2D) rectangle into which a 3D scene is projected.

The important thing to realize is that the viewport is not the same thing as the Canvas element.

The canvas is used to position the viewport on the page, and normally (by default) the viewport is set to be the full size of the canvas. However, it doesn't have to be - you can use renderer.setViewport to position the viewport rectangle anywhere within the canvas.

Also, the viewport is always specified using pixels as the unit (I don't know whether that's just a convention or if it's in a specs doc somewhere but I did some poking around and this always seems to be the case for OpenGL/WebGL/DirectX) - so, again, R3F specifying it in three.js units is non-standard.

@looeee
Copy link
Author

looeee commented Dec 12, 2021

when you want to set the viewport that is possible as well but it would be your responsibility to inform the state model. there is > a setSize function for that: master/packages/fiber/src/core/store.ts#L91 you would use that for the render function as well

const setSize = useThree(state => state.setSize)
  ...
  setSize(width, height)

The problem here is that setSize internallly will call renderer.setSize and that results in the actual canvas getting resized. I tried adding it to the sandbox above and weird things happen. When you change the viewport using renderer.setViewport you specifically don't want to resize the canvas.

Canvas resizing is expensive as it can cause browser layout recalculation, while viewport resizing is essentially free.

Here's a use-case for .setViewport: suppose I want a full screen scene with a sliding sidebar like this one. The blue area is the canvas:

full

Then I open the sidebar and it will cover part of the canvas:

partial

What if I want to shrink down the scene as the sidebar opens? So I want to animate the viewport/scene size from 100%vw, 100%vh to 70%vw, 100%vh over ~1 second. Animating the canvas size will work but it's probably going to be janky as hell due to all the layout recalculation. .setViewport is perfect here - I can resize the scene however I like without causing layout shifts while keeping the canvas full screen - in other words, I can call .setViewport every frame to shrink or grow the scene how I like, without worrying about performance.

@drcmda
Copy link
Member

drcmda commented Dec 12, 2021

ah yes, that was what i would have suggested but you're right, this will cause layout and setViewport is faster. this is the function that gets called when size/viewport changes (the state objects): https://github.com/pmndrs/react-three-fiber/blob/master/packages/fiber/src/core/store.ts#L335-L361

any ideas how that can be cleanly separated?

maybe shifting this bit:

      // Update renderer
      gl.setPixelRatio(viewport.dpr)
      gl.setSize(size.width, size.height)

one line up so that it's part of the closure that would allow users to bail out?

      // Do not mess with the camera if it belongs to the user
      if (!camera.manual && !(internal.lastProps.camera instanceof THREE.Camera)) {

that would be quite the breaking change but i guess manual is rarely used. but it could pour the entire thing into your own hands. though i would suggest v8. react 18 is available as a stable release candidate and v8 is only formally beta, it's probably more stable than v7 with all the bugfixes in there.

btw, im sure this will also mess with events and raycasting.

@CodyJasonBennett
Copy link
Member

I see two issues here:

  • viewport isn't representative of the gl viewport/drawbuffer size
  • updating the drawbuffer size independently of the DOM size; again, they're separate

IMO it's best to keep that responsibility to the renderer and abstract over that if needed, but this just seems to be unfortunate naming if that's not the intent behind viewport. Is this even needed in the first place? What's a use-case for having the viewport in three units and is there any dependency in the core?

@CodyJasonBennett
Copy link
Member

What's the resolution here? Can we use a name like container to consolidate information about the canvas container or move the three/viewport units elsewhere? I think it's confusing to reduce viewport to OpenGL semantics (R3F shouldn't be coupled to a back-end, and WebGPU/D3D12 semantics will only make this more confusing), but the term is still ambiguous here since it could also refer to the window, so I'd like to avoid it entirely.

@CodyJasonBennett CodyJasonBennett added the documentation to do with docs label Sep 3, 2022
@looeee
Copy link
Author

looeee commented Sep 8, 2022

I think it's confusing to reduce viewport to OpenGL semantics (R3F shouldn't be coupled to a back-end, and WebGPU/D3D12 semantics will only make this more confusing)

That's fair, but it's not just OpenGL semantics to use viewport with this meaning.

OpenGL Viewport

DirectX Viewport (previously linked above)

Not 100% sure about WebGPU but they do talk about "viewport coordinates" in the spec and mention that the coordinate system is the same as DirectX, so presumably Viewport has the same meaning there.

Vulkan Viewport

Metal Viewport

Not sure if there are any other relevant APIs but in any case, what I'm getting at with the list is that "Viewport" has a specific meaning in computer graphics literature. Using it in a different sense, especially a similar but functionality different sense as R3F is currently doing, is confusing.

I'd like to avoid it entirely.

That seems like a reasonable option.

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Sep 8, 2022

I think we're in agreement, but maybe for different reasons.

To clarify my comment, yes, all graphics APIs define a viewport, and the use of the term is consistent throughout. My concern is that they all define it differently, especially with coordinates, and R3F can't make any assumptions about the environment it's running in. This is why I bring up WebGL and WebGPU for WebGLRenderer and WebGPURenderer, respectively, but some renderers like SVGRenderer don't track or expose viewport information.

For those reasons, I don't think we can fix the measurements in state.viewport; only remove or rename it. If we do re-use the term for other reasons, it shouldn't exist at the top-level of RootState but properly scoped under state.container.viewport or wherever it is relevant.

I do question the need for getCurrentViewport in the first place, which is what state.viewport currently describes. It measures the canvas container in viewspace, but it's unclear why it exists in R3F. @drcmda, is this leftover from a refactor, and is this still useful?

@looeee
Copy link
Author

looeee commented Sep 8, 2022

My concern is that they all define it differently, especially with coordinates, and R3F can't make any assumptions about the environment it's running in

I'm not sure if that's true. Checking through the links I posted above, it's: viewport = (xOffset, yOffset, width, height). Then all except OpenGL also include minZ and maZ (for OpenGL that's stored as camera clipping planes instead).

The x and y offsets, in WebGL terms, are the offsets into the <Canvas> we are drawing into.

some renderers like SVGRenderer don't track or expose viewport information

Actually SVG does have a viewport although not sure if it's exposed through the three.js SVGRenderer. It's called a viewBox and it's defined in much the same way as the OpenGL viewport:

The value of the viewBox attribute is a list of four numbers: min-x, min-y, width and height. The numbers, which are separated by whitespace and/or a comma, specify a rectangle in user space which is mapped to the bounds of the viewport established for the associated SVG element (not the browser viewport).

However that highlights that viewport also has a meaning in the DOM and it's not the same as the graphics viewport. So it is an overloaded term when doing graphics on the web, unfortunately. I guess that is why SVG calls it a viewBox, but other APIs don't have the luxury of renaming basic terms to suit web browsers.

Anyway: the SVG element defines a viewport (web terminology), and the viewBox defines a viewport (graphics terminology) that can be offset into the SVG element.

Likewise in WebGL the <Canvas> defines a viewport (web terminology) and then the WebGLRenderingContext.viewport(xOffset, yOffset, width, height) defines a viewport (graphics terminology) that can be offset into the canvas viewport.

I do question the need for getCurrentViewport in the first place, which is what state.viewport currently describes

There is a need to get the WebGLRenderer.viewport but that doesn't have to be via a special R3F method.
My main concern here is R3F further overloading an already overloaded term. If that gets resolved in some way I am satisfied :)

Suggestions:

  1. Adopt the graphics version of viewport = (xOffset, yOffset, width, height), data taken from WebGLRenderer.viewport or equivalent in other APIs (if there's no way to get that, set xOffset = yOffset = 0 and width, height = canvas width, height.
  2. Adopt the web version of viewport. No offsets, width and height = canvas width, height.
  3. Avoid the term completely.

My preference is 1 but it's more complex to implement so I understand if you guys prefer another option.

@CodyJasonBennett
Copy link
Member

I'm not sure if that's true. Checking through the links I posted above, it's: viewport = (xOffset, yOffset, width, height). Then all except OpenGL also include minZ and maZ (for OpenGL that's stored as camera clipping planes instead).

It still doesn't look like I clarified why I bring this up for WebGL and WebGPU, specifically, but I'm going to drop it for the sake of closing this issue. I trust you don't mean to come across as contrarian or pedantic, but I find continued discussion in this regard distracting.

There is a need to get the WebGLRenderer.viewport but that doesn't have to be via a special R3F method.
My main concern here is R3F further overloading an already overloaded term. If that gets resolved in some way I am satisfied :)

Suggestions:

  1. Adopt the graphics version of viewport = (xOffset, yOffset, width, height), data taken from WebGLRenderer.viewport or equivalent in other APIs (if there's no way to get that, set xOffset = yOffset = 0 and width, height = canvas width, height.
  2. Adopt the web version of viewport. No offsets, width and height = canvas width, height.
  3. Avoid the term completely.

My preference is 1 but it's more complex to implement so I understand if you guys prefer another option.

I doubt the usefulness of 1 since we cannot rely on state.gl.viewport and always fallback to info derived from the canvas -- R3F supports any renderer that implements a render method, so there's no guarantee that state.gl.viewport is defined. We already track 2 in state.size, but that includes the canvas container's offsets (#2339). I don't think we can make any suggestions but 3 work if we absolutely cannot store additional information for 2.

@looeee
Copy link
Author

looeee commented Sep 9, 2022

I trust you don't mean to come across as contrarian or pedantic.

Not in the slightlest. Apologies if I came across that way, it was unintended.

I don't think we can make any suggestions but 3 work if we absolutely cannot store additional information for 2.

That's fine with me.

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Sep 9, 2022

@drcmda, it seems that the intention for the use of three units was to be able to create UI in threejs scaled to the container? If factor was measured camera view-space/container size, can't you can cleanly calculate it from Size?

const size = useThree(state => state.size)
const { factor } = useThree(state => state.viewport)

<planeGeometry args={[size.width, size.height]} scale={factor} />

IIRC Size best describes the canvas container viewport, with offsets introduced in #2339. What if we moved that to state.viewport but kept the relevant aspect, distance, and factor properties? I'm not sure if there's a preference for x and y, but left and top seem to be consistent when measuring in the DOM.

interface Viewport {
  /** Container horizontal offset in pixels. */
  left: number
  /** Container vertical offset in pixels. */
  top: number
  /** Container width in pixels. */
  width: number
  /** Container height in pixels. */
  height: number

  /** Container viewport aspect ratio. */
  aspect: number
  /** Camera distance from the origin in world space. */
  distance: number
  /** Camera view-space / container viewport ratio. */
  factor: number
}

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Jun 12, 2023

I'm looking to act on this in v9 (#2338), but I'm inclined to remove this property entirely and move the current behavior to a properly named helper method elsewhere. The current behavior measures in world-space which is wrong by convention. My previous comment looked to merge it with size for more correct behavior, but I don't see a need to use a previously problematic name and cause churn. People should be deferring to three.js for this information if critical, and with mrdoob/three.js#26160 it will always be correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core target agnostic (normally to do with reconciler) documentation to do with docs
Projects
None yet
Development

No branches or pull requests

3 participants