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

Additional WebXR image tracking changes for native integration #12176

Merged
merged 10 commits into from
Apr 11, 2022

Conversation

Alex-MSFT
Copy link
Contributor

@Alex-MSFT Alex-MSFT commented Mar 16, 2022

This PR includes some more changes for WebXR Image Tracking integration for BabylonNative. and some cleanup from the previous change: #12097

The primary change is to update nativeXRFrame to pull the results array directly from the frame object rather than marshalling back to native code every time. This should improve per-frame CPU load.

See: BabylonJS/BabylonNative#1032 for the consuming BabylonNative change.

…k since this is already gated on getImageTrackingResults and getTrackedImageScores, and change way we read in getImageTrackingResults to reduce native marshalling.
# Conflicts:
#	src/Engines/engine.ts
#	src/Engines/nativeEngine.ts
#	src/XR/features/WebXRImageTracking.ts
#	src/XR/native/nativeXRFrame.ts
@Alex-MSFT Alex-MSFT marked this pull request as draft March 16, 2022 21:23
@Alex-MSFT Alex-MSFT changed the title Additional WebXR Image Tracking for Native Integration Additional WebXR image tracking changes for native integration Mar 16, 2022
@Alex-MSFT Alex-MSFT marked this pull request as ready for review March 29, 2022 18:29
@Alex-MSFT Alex-MSFT marked this pull request as draft March 29, 2022 18:34
@azure-pipelines
Copy link

We have noticed you haven't changes the "what's new.md" file. If your update is important (a bug fix, a new feature), please make sure to update the what's new file in the base directory and commit the changes.

@azure-pipelines
Copy link

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

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

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

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

To test the snapshot in the playground itself use (for example):

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

# Conflicts:
#	packages/dev/core/src/XR/features/WebXRImageTracking.ts
@azure-pipelines
Copy link

We have noticed you haven't changes the "what's new.md" file. If your update is important (a bug fix, a new feature), please make sure to update the what's new file in the base directory and commit the changes.

@azure-pipelines
Copy link

We have noticed you haven't changes the "what's new.md" file. If your update is important (a bug fix, a new feature), please make sure to update the what's new file in the base directory and commit the changes.

@azure-pipelines
Copy link

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

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

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

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

To test the snapshot in the playground itself use (for example):

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

@azure-pipelines
Copy link

We have noticed you haven't changes the "what's new.md" file. If your update is important (a bug fix, a new feature), please make sure to update the what's new file in the base directory and commit the changes.

@azure-pipelines
Copy link

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

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

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

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

To test the snapshot in the playground itself use (for example):

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

@Alex-MSFT Alex-MSFT marked this pull request as ready for review April 4, 2022 17:50
@RaananW
Copy link
Member

RaananW commented Apr 6, 2022

Is this ready to be merged now?

@Alex-MSFT
Copy link
Contributor Author

Is this ready to be merged now?

I believe so, as long as you are okay with taking out the IsCompatible check for turning this feature on. Basically the only cost there is we will load the image for cases where it might not be necessary.

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.

Just a question about back compat, otherwise LGTM!

packages/dev/core/src/Engines/engine.ts Show resolved Hide resolved
@bghgary
Copy link
Contributor

bghgary commented Apr 7, 2022

@Alex-MSFT Just double checking, we don't need to update the protocol version for this right?

@Alex-MSFT
Copy link
Contributor Author

@Alex-MSFT Just double checking, we don't need to update the protocol version for this right?

No we should not need to, this change should be forwards compatible with the current BabylonNative without any changes.

@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@azure-pipelines
Copy link

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

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

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

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

To test the snapshot in the playground itself use (for example):

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

@bghgary bghgary merged commit a578406 into BabylonJS:master Apr 11, 2022
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.

4 participants