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.

Sign up for GitHub

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

Jump to bottom

Implement Lazy Picking for POINTERMOVE #13044

Merged
merged 29 commits into from
Oct 17, 2022
Merged

Implement Lazy Picking for POINTERMOVE #13044

merged 29 commits into from
Oct 17, 2022

Conversation

PolygonalSun
Copy link
Contributor

@PolygonalSun PolygonalSun commented Sep 29, 2022 •
edited
Loading

This PR changes the code for picking during a move event to only pick when the user (or mechanism like the ActionManager) require it.

@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

Snapshot stored with reference name:
refs/pull/13044/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13044/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13044/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/13044/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/13044/merge
https://gui.babylonjs.com/?snapshot=refs/pull/13044/merge
https://nme.babylonjs.com/?snapshot=refs/pull/13044/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/13044/merge#BCU1XR#0

bghgary
bghgary reviewed Oct 3, 2022
View reviewed changes
Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

Not many big concerns, but there is quite a bit of new code introduced when switching cameras from input manager to device source manager. Is that expected?

packages/dev/core/src/Cameras/Inputs/BaseCameraMouseWheelInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraMouseWheelInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraPointersInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraPointersInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraPointersInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraPointersInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/scene.ts Show resolved Hide resolved
packages/dev/core/src/scene.ts Outdated Show resolved Hide resolved
bghgary
bghgary reviewed Oct 6, 2022
View reviewed changes
Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

Some minor issues, but overall, looking good.

packages/dev/core/src/Cameras/Inputs/BaseCameraMouseWheelInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraMouseWheelInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraPointersInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/DeviceInput/gestureRecognizer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/DeviceInput/gestureRecognizer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/DeviceInput/gestureRecognizer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/DeviceInput/gestureRecognizer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/BaseCameraPointersInput.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Cameras/Inputs/arcRotateCameraMouseWheelInput.ts Outdated Show resolved Hide resolved
bghgary
bghgary reviewed Oct 12, 2022
View reviewed changes
packages/dev/core/src/Collisions/pickingInfo.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Events/pointerEvents.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Culling/ray.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
@PolygonalSun PolygonalSun changed the title Implement Lazy Picking for POINTERMOVE and modify cameras to work with DeviceSourceManager Implement Lazy Picking for POINTERMOVE Oct 14, 2022
bghgary
bghgary approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

LGTM

@PolygonalSun PolygonalSun marked this pull request as ready for review October 14, 2022 23:16
sebavan
sebavan reviewed Oct 14, 2022
View reviewed changes
packages/dev/core/src/Actions/actionManager.ts Show resolved Hide resolved
packages/dev/core/src/Culling/ray.ts Show resolved Hide resolved
packages/dev/core/src/Events/pointerEvents.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Events/pointerEvents.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Show resolved Hide resolved
packages/dev/core/src/Inputs/scene.inputManager.ts Outdated Show resolved Hide resolved
packages/dev/core/src/scene.ts Show resolved Hide resolved
packages/dev/core/src/scene.ts Show resolved Hide resolved
@PolygonalSun PolygonalSun enabled auto-merge (squash) October 17, 2022 22:24
@PolygonalSun PolygonalSun merged commit 61bcae7 into BabylonJS:master Oct 17, 2022
@PolygonalSun PolygonalSun mentioned this pull request Nov 14, 2022
Closed
RaananW pushed a commit that referenced this pull request Dec 9, 2022
@PolygonalSun
Implement Lazy Picking for POINTERMOVE (#13044)
c833af2 
* Lazy Pick on Move

* Change to account for undefined

* cameras using DSM

* Check for reasons to pick before picking

* Addressed some feedback (missing meshUnderPointer)

* Modified get meshUnderPointer

* Add frame awareness picking for move

* Caching Test

* format

* import fix

* Removed test code

* PR Feedback

* fixed import

* removed gesture recognizer and reverted inputs

* Revert videoDome because it uses picking info

* formatting

* Changed to just MOVE Lazy Picking

* Format

* Removed comment

* PR Feedback

* Re-added meshunderpointer code back

* More feedback

* Comment

* PR Feedback part 1

* comment

Former-commit-id: 41bb2ae6975b3373d6f111e8b13d8261f1034569
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebavan sebavan sebavan approved these changes

@bghgary bghgary bghgary approved these changes

@ryantrem ryantrem Awaiting requested review from ryantrem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PolygonalSun @sebavan @bghgary