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

Handle detached arrays in XRView #26153

Closed
Manishearth opened this issue Apr 8, 2020 · 13 comments
Closed

Handle detached arrays in XRView #26153

Manishearth opened this issue Apr 8, 2020 · 13 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Apr 8, 2020

There are a bunch of places in the WebXR spec where arrays are returned. If you call .transfer() on these, their buffers are "detached", and need to be recomputed. You can check if a buffer is detached by calling is_null() on it.

We handle most of these cases, except for XRView.projectionMatrix

We need to store the projection matrix on the XRView and use a null check in ProjectionMatrix to recompute it if necessary:

NonNull::new(self.proj.get()).unwrap()

@highfive
Copy link

@highfive highfive commented Apr 8, 2020

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 13, 2020

Hi! I wanted to get started with contributing to servo and this issue was listed on Servo Starters. I am familiar with rust and I want to work on this issue.

We handle most of these cases, except for XRView.projectionMatrix

Can you link me with a example where this case is already being handled, it will help me figure out what exactly I need to implement here!

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 13, 2020

@daemon1024 in all the other cases the array is lazy initialized so fetching it when it's null after detachment just reinitializes it

fn Matrix(&self, _cx: JSContext) -> NonNull<JSObject> {

@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 13, 2020

@Manishearth Thanks for reference. Got it now, will get started to work on it.

@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 13, 2020

@highfive: assign me

@highfive highfive added the C-assigned label Apr 13, 2020
@highfive
Copy link

@highfive highfive commented Apr 13, 2020

Hey @daemon1024! Thanks for your interest in working on this issue. It's now assigned to you!

@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 13, 2020

From what I understood I need to initialise self.proj using create_typed_array with the underlying view's projection matrix as source.

Underlying view can be referenced as self.view but no field of proj/projectionmatrix/projections exists in self.view.

Also view field in the struct XRView is Heap here, I think it should be of the type webxr_api::View for me to be able to reference the property.

@Manishearth ,can you assist me with where am I going wrong?

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 13, 2020

Yes, you need to copy the projection matrix into the XRView as a new field during construction.

You still need the Heap to be able to make the array.

@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 13, 2020

Logically it should be something like this i guess

if self.proj.get().is_null() {
let cx = self.global().get_cx();
let projsrc = self.view.projection;
create_typed_array(cx, &projsrc, &self.proj);
}

But I can't seem to figure how access the projection matrix source ( projsrc here ) that i need to copy.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 13, 2020

You have view in new() during construction, you should copy out view.projection and store it on a field.

@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 14, 2020

I added a new field as u suggested

pub struct XRView {
....
viewsrc: &'static [f32],
}

I tried to store it out from new() using

let projm = view.projection.to_row_major_array();
let ret = reflect_dom_object(
Box::new(XRView::new_inherited(session, &transform, eye, &projm )), global, );

But I am getting an error

borrowed value does not live long enough
| cast requires that projm is borrowed for 'static

I am not sure how else do I copy it out.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 14, 2020

It needs to be a Vec<f32>, you can't hold on to borrowed slices there

@daemon1024 daemon1024 mentioned this issue Apr 14, 2020
3 of 5 tasks complete
@daemon1024
Copy link
Contributor

@daemon1024 daemon1024 commented Apr 14, 2020

@Manishearth Thank You so much for the help, I have made the required changes and made a PR. Can you please review it?

bors-servo added a commit that referenced this issue Apr 15, 2020
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 15, 2020
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 15, 2020
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 15, 2020
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.