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

fix(RTTR): set initial size for NaN in viewport #3137

Merged

Conversation

AbsharHassan
Copy link
Contributor

Overview

This PR addresses issue #3133. It includes two main changes:

  1. Bug Fix: Corrects the error in the viewport and camera objects by explicitly creating the size object in the configure function to ensure proper passing of width and height properties.
  2. Test Update: Enhances the test for the useThree hook to accurately reflect and validate the size object's structure, using specific dimensions to improve test reliability.

These changes ensure that the configure function correctly handles size properties, and the updated test verifies this functionality effectively.

Process

I began by inspecting the source files for the fiber package. I noticed that the configure function (returned by the createRoot function) requires the desired width and height (among other size attributes) to be within a size object. It does not accept width and height directly.

Upon inspecting source files for the test-renderer package, I noticed that the structure of the props were being passed to the configure function like so:

const _root = createRoot(canvas).configure({ frameloop: 'never', ...options, events: undefined })

given that the docs show that the structure of the createOptions prop is

// RenderProps is from react-three-fiber
interface CreateOptions extends RenderProps<HTMLCanvasElement> {
  width?: number // width of canvas
  height?: number // height of canvas
}

This is not in agreement with the observation that, essentially, width and height should be part of a size object.

Potential Fixes:

  1. The most simple fix would be to update the docs, such that the end-user explicitly passes a size object, containing the desired width and height, ALONG with top and left, as the second parameter of the create function, like so:
const renderer = ReactThreeTestRenderer.create(element, {size: {/* desired width and height*/ })

however, this approach might cause issues with existing tests written with RTTR, which I believe may not be ideal. Moreover, it will require typescript users to explicitly pass top and left props, as type Size requires top and left to be defined.

  1. Another fix I came up with was to explicitly create a size object containing the user input width and height attributes, along with other default required attributes, in the create function and also passing any other props that may be added in the future. The code is:
  let size

  if (typeof options !== 'undefined' && typeof options.width !== 'undefined' && typeof options.height !== 'undefined') {
    size = { width: options.width, height: options.height, top: 0, left: 0 }
  }

  const _root = createRoot(canvas).configure({
    frameloop: 'never',
    ...options,
    size,
    events: undefined,
  })

with this fix, no changes will be needed in the way the api is used, however the code is slightly complex and redundant. It may serve as a workable fix until the maintainers see fit to release a new version for the api.

This PR is implementing the 2nd fix.

Results

The original issue is resolved. The NaN values are gone and the viewport is calculated as expected. Moreover, the NaN values in camera are also resolved. Here is an example result for the viewport, if the width is set to 1280 and height is set to 800

{
    initialDpr: 1,
    dpr: 1,
    width: 12.277231807663368,
    height: 7.673269879789604,
    top: 0,
    left: 0,
    aspect: 1.6,
    distance: 5,
    factor: 104.25802982729644,
    getCurrentViewport: [Function: getCurrentViewport]
}

The test for the useThree() hook was also updated to reflect the correct calculation of the size object.

Feedback and suggestions for improvements are welcome. @CodyJasonBennett, your guidance on enhancing this PR would be greatly appreciated.

This commit resolves the issue where width and height properties were not properly structured as part of a size object in the configure function, causing errors in viewport calculation. Now, the function correctly creates and includes a size object with width, height, top, and left properties, ensuring accurate viewport handling as well as camera handling.

Closes pmndrs#3133
Updated the test for the useThree hook to accurately check the size property. The test now uses specific dimensions (width: 1280, height: 800) to create the component, ensuring that the size object is correctly tested against expected values. This change improves the test's effectiveness in catching potential size-related issues in the configure function.

Related to pmndrs#3133
Copy link

codesandbox-ci bot commented Jan 2, 2024

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 ed2c0e3:

Sandbox Source
example Configuration

@CodyJasonBennett CodyJasonBennett changed the title Fix and Test for Issue #3133: Correct Prop Passing to Configure Function fix(RTTR): set initial size for NaN in viewport Jan 2, 2024
@CodyJasonBennett CodyJasonBennett linked an issue Jan 2, 2024 that may be closed by this pull request
@CodyJasonBennett CodyJasonBennett merged commit 6d733ed into pmndrs:master Jan 2, 2024
2 checks passed
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.

RTTR: NaN values in state.viewport
2 participants