Skip to content

Support extern context impl #6658

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

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Conversation

sagudev
Copy link
Collaborator

@sagudev sagudev commented Dec 4, 2024

Connections
Based on #6619, fixes #6330

Description
This PR enables users of wgpu to provide custom Instance impl (and other wgpu types as all other types are derived from Instance->Adapter->Device), by adding Custom variant and custom backend that has dyn wrapper for all types: struct DynDevice(Arc<dyn DeviceInterface>). I'm intermixing Custom with Dyn because I cannot really decide on name (CustomDevice is weird, but we already have Dyn* in wgpu-hal).
There are few rough sports:

  • RenderBundleEncoderInterface::finish (other backends hacks around by not using dyn dispatch, but we can't:
    pub fn finish(self, desc: &RenderBundleDescriptor<'_>) -> RenderBundle {
    let bundle = match self.inner {
    #[cfg(wgpu_core)]
    dispatch::DispatchRenderBundleEncoder::Core(b) => b.finish(desc),
    #[cfg(webgpu)]
    dispatch::DispatchRenderBundleEncoder::WebGPU(b) => b.finish(desc),
    };

Testing
There is wgpu-custom crate that showcases how one can create Custom wgpu types.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@sagudev sagudev requested a review from a team as a code owner December 4, 2024 19:50
@sagudev sagudev marked this pull request as draft December 4, 2024 19:50
@sagudev sagudev changed the title Init CustomContext and Dyn_ WIP: SUpport extern context impl Dec 4, 2024
@sagudev sagudev changed the title WIP: SUpport extern context impl WIP: Support extern context impl Dec 4, 2024
@cwfitzgerald cwfitzgerald self-assigned this Dec 5, 2024
@sagudev
Copy link
Collaborator Author

sagudev commented Dec 5, 2024

TBH, I have no idea what's wrong with WASM.

Comment on lines 207 to 209
#[doc(hidden)]
/// Creates Instance from custom context implementation
pub fn from_custom_instance(instance: std::sync::Arc<dyn InstanceInterface>) -> Self {
Self {
inner: dispatch::DispatchInstance::Custom(backend::custom::DynContext::new(instance)),
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is triggering visibility problems for WASM, but I do not know why.

@sagudev sagudev force-pushed the custom-ctx branch 2 times, most recently from 9090201 to 088532e Compare December 6, 2024 09:07
@cwfitzgerald
Copy link
Member

Ping me if this needs looking at or you need help before it is out of draft, going to un-assign myself for bookeeeping

@sagudev
Copy link
Collaborator Author

sagudev commented Jan 8, 2025

In 79ecded I removed InterfaceTypes trait as it's not really needed anymore (this makes writing Custom/Dyn wrappers easier as they do not need to impl traits directly, avoiding a lot of boilerplate), all bounds are actually enforced in dispatch_types_inner anyway. The only useful application was making writing macros easier, but I replaced this with mod interface_types that has aliases for interface types (there is a lot of this pattern in servo as it was used before associate trait types), but we could also just write more stuff in macro and remove the completely. Does approach like this sounds reasonable @cwfitzgerald? I would be willing to split this into separate PR if it's ok.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jan 8, 2025

Hmm... I don't love it as it makes things less structured, but can't see a strong argument against removing the trait. Will fully consider it when I review this. You can leave it as part of this PR for now.

@sagudev
Copy link
Collaborator Author

sagudev commented Jan 9, 2025

I have alternative approach in sagudev@ce58280 which keeps the trait but instead of using interfaces in bounds it uses Dispatch<Target = dyn Interface> (clone of Deref trait, I avoid Deref to avoid compiler magic as it makes bounds less explicit esp between Dispatch and DispatchMut). One thing to consider here is that implementations of custom context will need to provide all types, even those that are not needed (ex. if one does not need/use compute pass). Should I switch to this approach @cwfitzgerald?

@cwfitzgerald
Copy link
Member

I have alternative approach in sagudev@ce58280 which keeps the trait but instead of using interfaces in bounds it uses Dispatch<Target = dyn Interface> (clone of Deref trait, I avoid Deref to avoid compiler magic as it makes bounds less explicit esp between Dispatch and DispatchMut).

I'm going to leave this up to your own judgement which is cleaner and more parsable, my initial impression is that the mod { type } approach is less infrastructure and less confusing than adding the Dispatch one

@sagudev sagudev force-pushed the custom-ctx branch 2 times, most recently from cf170b3 to d693c1b Compare January 29, 2025 18:58
@sagudev sagudev changed the title WIP: Support extern context impl Support extern context impl Jan 29, 2025
@sagudev
Copy link
Collaborator Author

sagudev commented Jan 29, 2025

@cwfitzgerald I think this is ready for concept review, it still needs more docs and maybe some polish 💅🏾.

@cwfitzgerald
Copy link
Member

Nice! I'm going to take this out of draft for bookkeeping

@cwfitzgerald cwfitzgerald marked this pull request as ready for review January 29, 2025 21:39
@cwfitzgerald cwfitzgerald self-assigned this Feb 5, 2025
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.

Don't love the whole new type alias thing, but don't really hate it enough to say no

@sagudev
Copy link
Collaborator Author

sagudev commented Feb 13, 2025

Don't love the whole new type alias thing, but don't really hate it enough to say no

I think we could actually inline all types uses into macro invocation (all 3 of them). Would that be better?

@cwfitzgerald
Copy link
Member

I think we could actually inline all types uses into macro invocation

Hmm, yeah I think that's fine - the whole appeal to me is to make sure things have uniform naming conventions and are all filled out, so having them all in one place seems to do that trick fine.

@sagudev
Copy link
Collaborator Author

sagudev commented Feb 15, 2025

I think we could actually inline all types uses into macro invocation

Hmm, yeah I think that's fine - the whole appeal to me is to make sure things have uniform naming conventions and are all filled out, so having them all in one place seems to do that trick fine.

I am not sure I understand, should I inline all types uses into macro or keep the mods?

@cwfitzgerald
Copy link
Member

Oh sorry was unclear, inline into the macro.

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.

Some small things, but I think this is good to go after that!

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.

I have some futzing I want to do to the examples, but I'll do that post land.

Thanks!

@cwfitzgerald cwfitzgerald merged commit 7e66495 into gfx-rs:trunk Mar 4, 2025
34 checks passed
@DJMcNab
Copy link
Collaborator

DJMcNab commented Apr 17, 2025

Has this been released? I'm trying to check the changelog for when this got released

@sagudev
Copy link
Collaborator Author

sagudev commented Apr 17, 2025

Has this been released? I'm trying to check the changelog for when this got released

Yes it was, but there are still some rough edges: #7533

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.

Support extern Context implementations
3 participants