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

Implement hit testing API #26171

Merged
merged 18 commits into from
Apr 20, 2020
Merged

Implement hit testing API #26171

merged 18 commits into from
Apr 20, 2020

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Apr 11, 2020

Depends on servo/webxr#149 , #26170

This implements non-transient hit tests.

The tests that do not pass are due to web-platform-tests/wpt#22898 , web-platform-tests/wpt#22900, web-platform-tests/wpt#22901 , and immersive-web/hit-test#86

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/XRSession.webidl, components/script/dom/webidls/XRRay.webidl, components/script/canvas_state.rs, components/script/dom/xrreferencespace.rs, components/script/dom/mod.rs and 21 more
  • @KiChjang: components/script/dom/webidls/XRSession.webidl, components/script/dom/webidls/XRRay.webidl, components/script/canvas_state.rs, components/script/dom/xrreferencespace.rs, components/script/dom/mod.rs and 21 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 11, 2020
@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#22868.

@Manishearth
Copy link
Member Author

I haven't gone through all the tests but the reason we fail the refSpace one, for example, is both because of immersive-web/hit-test#86 and because of another bug in the test where it's assuming hit test results get duplicated

@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#22868.

@servo-wpt-sync
Copy link
Collaborator

No upstreamable changes; closed existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#22868.

@Manishearth
Copy link
Member Author

Rebased over test updates that I landed upstream early.

@jdm
Copy link
Member

jdm commented Apr 14, 2020

r? @asajeffrey

Copy link
Member

@asajeffrey asajeffrey left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, we're generating a lot of objects per frame, but hey ho.

One thing I don't understand is how to do hit tests relative to a moving refererence, e.g. from the users's hands or eyes? Does this require generating a new ray each frame?

.hit_test_results
.iter()
.filter(|r| r.id == source.id())
.map(|r| XRHitTestResult::new(&self.global(), *r, self))
Copy link
Member

Choose a reason for hiding this comment

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

Ouch, creating a new hit test result for each hit test, every frame? This might be putting strain on the GC?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any other way to do it? Note that it's possible for a single hit test to have multiple results.

fn GetPose(&self, base: &XRSpace) -> Option<DomRoot<XRPose>> {
let base = self.frame.get_pose(base)?;
let pose = base.inverse().pre_transform(&self.result.space);
Some(XRPose::new(&self.global(), pose.cast_unit()))
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, creating a new XRPose object every frame?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already do this with getPose in views

@CYBAI
Copy link
Member

CYBAI commented Apr 16, 2020

Needs a ./mach fmt 👀

Diff in /repo/components/script/dom/xrsession.rs at line 747:
             .granted_features()
             .iter()
             .find(|f| &**f == "hit-test")
-            .is_none() {
�(B+            .is_none()
�(B+        {
�(B             p.reject_error(Error::NotSupported);
             return p;
         }
clang-format not installed. Skipping CPP formatting.
Run `./mach fmt` to fix the formatting

@Manishearth
Copy link
Member Author

Strangely I do not get that error locally

@Manishearth
Copy link
Member Author

That patch is also already applied here and on github. I suspect tc is doing something weird.

(tc won't pass anyway until servo/webxr#149 lands)

@CYBAI
Copy link
Member

CYBAI commented Apr 17, 2020

Checking files for tidiness...
./Cargo.lock:1: duplicate versions for package `autocfg`
	The following packages depend on version 0.1.7 from 'crates.io':
	The following packages depend on version 1.0.0 from 'crates.io':
./components/script/dom/webidls/XRHitTestResult.webidl:10: no newline at EOF

bors-servo added a commit to servo/webxr that referenced this pull request Apr 19, 2020
Implement support for hit testing, use in mock backend

Servo side: servo/servo#26171

This adds support for all non-transient parts of he [WebXR Hit Test Module](https://immersive-web.github.io/hit-test), including mocking support.

This has not yet been hooked up to script and is thus completely untested.

r? @jdm
@Manishearth
Copy link
Member Author

@bors-servo try

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Apr 20, 2020
@Manishearth
Copy link
Member Author

Ah, our webxr tests need to be updated for supportedModes

@Manishearth
Copy link
Member Author

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

📌 Commit 2a57966 has been approved by asajeffrey

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 20, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit 2a57966 with merge 575c8ed...

bors-servo added a commit that referenced this pull request Apr 20, 2020
Implement hit testing API

Depends on servo/webxr#149 , #26170

This implements non-transient hit tests.

The tests that do not pass are due to web-platform-tests/wpt#22898 , web-platform-tests/wpt#22900, web-platform-tests/wpt#22901 , and immersive-web/hit-test#86
@bors-servo
Copy link
Contributor

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 20, 2020
@jdm
Copy link
Member

jdm commented Apr 20, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit 2a57966 with merge 99cd30e...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 20, 2020
@bors-servo
Copy link
Contributor

☀️ Test successful - status-taskcluster
Approved by: asajeffrey
Pushing 99cd30e to master...

@bors-servo bors-servo merged commit 99cd30e into servo:master Apr 20, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 20, 2020
@Manishearth Manishearth deleted the hittest branch April 20, 2020 04:49
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.

7 participants