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

test(particles.cloudPoint): add tests for intersectsMesh function #12992

Merged
merged 1 commit into from Sep 19, 2022

Conversation

Dok11
Copy link
Contributor

@Dok11 Dok11 commented Sep 18, 2022

Hi!
I add some tests to Particles/cloudPoint, the function is intersectsMesh.

Here I found some points.

  1. The property of PointsCloudSystem mesh has incorrect type. In fact this property may have undefined as value, because it's not defined in the constructor. So I add optional mark ? to type and required checks in the class.
  2. And after this I found what the intersectsMesh function did not check the mesh property existing in the point cloud system. So I add a new Error here, that notice about required mesh to next calculations.
  3. The line isSphere = isSphere ? isSphere : false; look like redundant, because TypeScript promise that it have boolean type.
  4. Next I move the getBoundingInfo call into one line const bbox = target.getBoundingInfo().boundingBox;. It's look for me as potential performance boost, maybe.
  5. And at the end I remove else branch because it rendundant because the code above have return statements.

And I limited test cases from 729 possible combinations to 126, they seems like corner cases for me. I dont know which cases we can reduce to save the working guarantee.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

LGTM!

@RaananW RaananW merged commit d0d52bd into BabylonJS:master Sep 19, 2022
@Dok11 Dok11 deleted the test-babylon-cloud-point branch September 19, 2022 18:48
RaananW pushed a commit that referenced this pull request Dec 9, 2022
…2992)

Former-commit-id: 0fbcdf2523a7d23f9e6f77d8f403f753fdd6c8ad
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