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 support for hit testing, use in mock backend #149

Merged
merged 8 commits into from
Apr 19, 2020

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Apr 3, 2020

Servo side: servo/servo#26171

This adds support for all non-transient parts of he WebXR Hit Test Module, including mocking support.

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

r? @jdm

@@ -82,6 +84,14 @@ pub trait DeviceAPI<Surface>: 'static {
}

fn granted_features(&self) -> &[String];

fn request_hit_test(&mut self, _source: HitTestSource) {
panic!("This device does not support requesting hit tests");
Copy link
Member

Choose a reason for hiding this comment

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

Would a error! be more appropriate? I don't have an opinion on the matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

if this happens it's a bug on our side, it means we either wrote a backend that falsely claimed hit test support, or the script code is not validating features

which reminds me, the mock backend should start reporting that it supports hit testing

@@ -43,6 +45,19 @@ pub struct EntityTypes {
pub mesh: bool,
}

#[derive(Copy, Clone, Debug)]
#[cfg_attr(feature = "ipc", derive(serde::Serialize, serde::Deserialize))]
/// Vec<EntityType>, but better
Copy link
Member

Choose a reason for hiding this comment

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

orly?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a vec in the spec, but there are only three possible values, and it's easier to store and iterate in this form

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but look at the lines following the line that I commented on. I think you copied a comment from another file.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh oops, GitHub didn't show the context below lol

Comment on lines 214 to 215
for region in &world.regions {
if source.types.is_type(region.ty) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps:

let hits = world
    .regions
    .iter()
    .filter(|region| !source.types.is_type(region.ty))
    .flat_map(|region| &region.faces);
    .filter_map(|triangle| triangle.intersect(ray))
    .map(|space| HitTestResult { space, id: source.id });
frame.hit_test_results.extend(hits);

@Manishearth Manishearth force-pushed the hittest branch 2 times, most recently from d85f412 to 2ed26d4 Compare April 11, 2020 05:08
@Manishearth Manishearth marked this pull request as ready for review April 11, 2020 07:27
@Manishearth Manishearth changed the title [WIP] Implement support for hit testing, use in mock backend Implement support for hit testing, use in mock backend Apr 11, 2020
@Manishearth
Copy link
Member Author

(this is ready for final review, but we should only merge this once the servo side (servo/servo#26171) lands)

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.

Some niggles.

webxr-api/hittest.rs Show resolved Hide resolved
pub point: bool,
pub plane: bool,
pub mesh: bool,
}
Copy link
Member

Choose a reason for hiding this comment

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

use bitstring for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean bitflags? I suppose, this seemed to be pretty small.

Copy link
Member Author

Choose a reason for hiding this comment

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

Chose to not do this, the code is about as complicated either way.

frame.hit_test_results.extend(hits);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Hit tests are performed synchronously every frame? For triangles hit-testing is cheap, but is this going to be expensive for other hit tests, e.g. meshes?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the mock backend, we don't care. In openxr we'll mostly be passing it down to the system.

@Manishearth
Copy link
Member Author

@asajeffrey are you okay with this and the servo PR landing? I have more test changes to make and it would be easier to not build them on top of this

@asajeffrey
Copy link
Member

Yes

@Manishearth
Copy link
Member Author

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

📌 Commit b5a3f97 has been approved by asajeffrey

bors-servo added a commit 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
@bors-servo
Copy link
Contributor

⌛ Testing commit b5a3f97 with merge ab47b0b...

@bors-servo
Copy link
Contributor

💔 Test failed - checks-travis

@Manishearth
Copy link
Member Author

Seems like the googlevr backend has been busted on master since the upgrade, cc @asajeffrey

Going to manual merge

@Manishearth Manishearth merged commit 8058115 into servo:master Apr 19, 2020
@Manishearth Manishearth deleted the hittest branch April 19, 2020 18:44
bors-servo added a commit to servo/servo that referenced this pull request Apr 19, 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 added a commit to servo/servo 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 added a commit to servo/servo 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
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