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

More as_hal() methods #3966

Closed
wants to merge 47 commits into from
Closed

More as_hal() methods #3966

wants to merge 47 commits into from

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Jul 23, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

@JMS55
Copy link
Contributor Author

JMS55 commented Aug 1, 2023

Open question: How to ensure textures are kept alive while native/non-wgpu render work takes place?

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Minus nits, lets just get this in, we can improve the apis later.

wgpu-core/Cargo.toml Outdated Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved

/// # Safety
/// - The raw command encoder handle must not be manually destroyed
pub unsafe fn command_encoder_as_hal_mut<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to add the not wasm #cfg's here, and in a few other places?

Copy link
Member

Choose a reason for hiding this comment

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

We should

@@ -79,7 +79,7 @@ impl<A: hal::Api> CommandEncoder<A> {
}
}

fn open(&mut self) -> &mut A::CommandEncoder {
pub fn open(&mut self) -> &mut A::CommandEncoder {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did I need to make this method public, and CommandBuffer.encoder pub(crate)? Not sure.

@cwfitzgerald
Copy link
Member

Poke - need to rebase and fix CI.

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 15, 2023

I've rebased, but not quite sure what cfg's to add where. It seems inconsistent, not all existing as_hal methods have them, and I'm not sure which crates need it or not. Also, not sure what cfg to use - it seems like some of them allow webgl or emscripten? Those seem wrong, surely it's supposed to be not(any(wasm32, webgl, emscripten))?

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Sep 15, 2023

Both emscripten and webgl are wgpu-core/wgpu-hal based, so they should be allowed through. The intent is for it to be consistent, but I wouldn't be surprised if there were mistakes

@cwfitzgerald cwfitzgerald requested a review from a team September 21, 2023 19:05
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Look ma, a review

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Seem to have some CI issues.

@xiaopengli89
Copy link
Contributor

Can we add 2 more methods?

  • hal::Texture::raw_texture return the raw texture
  • hal::CommandEncoder::raw_command_buffer return the raw command buffer

@JMS55 JMS55 closed this Jan 3, 2024
@JMS55
Copy link
Contributor Author

JMS55 commented Jan 3, 2024

I don't have the time or motivation to work on this anymore (this was originally intended for use with DLSS, but I gave up on that idea). Ideally someone will take up #4067.

@JMS55 JMS55 mentioned this pull request Mar 29, 2024
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.

Allow hooking into command buffers on a backend-specific basis
3 participants