-
Notifications
You must be signed in to change notification settings - Fork 937
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
Improve locking safety for Registry #32
Conversation
To clarify, even though parts of this PR are to be scrapped, at least the change of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
Agreed, it would be good to fix local.
wgpu-bindings/Cargo.toml
Outdated
@@ -10,4 +10,5 @@ authors = [ | |||
default = [] | |||
|
|||
[dependencies] | |||
cbindgen = { git = "https://github.com/eqrion/cbindgen.git" } | |||
#cbindgen = "0.6.7" | |||
cbindgen = { git = "https://github.com/eqrion/cbindgen" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also use my branch for now if we're blocked by the header changes
@@ -7,7 +7,6 @@ use hal; | |||
|
|||
|
|||
bitflags! { | |||
#[repr(transparent)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is temporary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accidental...
wgpu-native/src/registry/remote.rs
Outdated
|
||
pub struct Items<T> { | ||
next_id: Id, | ||
next_id: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Id
-> u32
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor artifact :)
wgpu-native/src/registry/remote.rs
Outdated
@@ -35,11 +32,11 @@ impl<T> super::Items<T> for Items<T> { | |||
id | |||
} | |||
|
|||
fn get(&self, id: Id) -> super::Item<T> { | |||
fn get(&self, id: Id) -> & T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space
Thanks for the review! All the concerns should be addressed now. |
Build succeeded |
32: Add conversion matrix from OpenGL to wgpu r=kvark a=dragly The matrices in the examples are given in an OpenGL-like coordinate system, while a Vulkan-like coordinate system is used by wgpu. This was previously partially corrected in the shader and by flipping the up axis of the camera, but left the x-axis mirrored in the final result. This change adds a conversion matrix to framework.rs that can be used to convert from OpenGL to wgpu. This also allows us to set the winding-order to counter-clockwise, which matches the ordering in the data. Co-authored-by: Svenn-Arne Dragly <dragly@cognite.com> Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>
32: Add conversion matrix from OpenGL to wgpu r=kvark a=dragly The matrices in the examples are given in an OpenGL-like coordinate system, while a Vulkan-like coordinate system is used by wgpu. This was previously partially corrected in the shader and by flipping the up axis of the camera, but left the x-axis mirrored in the final result. This change adds a conversion matrix to framework.rs that can be used to convert from OpenGL to wgpu. This also allows us to set the winding-order to counter-clockwise, which matches the ordering in the data. Co-authored-by: Svenn-Arne Dragly <dragly@cognite.com> Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>
This isn't ideal: we shouldn't be mutably locking anything for adding/removing objects in
local
path. But at least it makes it safe, for now.