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

Multiple blade backends (Vulkan/Metal or GLES) #14931

Closed

Conversation

lukaslihotzki
Copy link
Contributor

This adds multiple blade backends to zed. Vulkan or Metal is used by default. The environment variable USE_GLES=1 can be set to switch to GLES. This depends on kvark/blade#148.

The implementation uses a file-based polymorphism approach, where the same file is included twice in the default and gles module by using the same path attribute. This approach was chosen, because blade does not expose everything as traits. Both backends expose a dyn BladeRenderer trait object, so there is little performance overhead, basically just one indirection on every draw scene call.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jul 21, 2024
@zed-industries-bot
Copy link

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- (Added|Fixed|Improved) ... ([#<public_issue_number_if_exists>](https://github.com/zed-industries/zed/issues/<public_issue_number_if_exists>)).

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against 1b8e6f4

@maxdeviant
Copy link
Member

The implementation uses a file-based polymorphism approach, where the same file is included twice in the default and gles module by using the same path attribute.

I think we’ll need to find an alternative approach that doesn’t rely on path if we intend to merge this.

The approach is too magical and comes at the cost of code clarity.

@lukaslihotzki
Copy link
Contributor Author

Other solutions for compiling the same code with different blade namespaces would be: Use symlinks or include! instead of path attribute (also magical) or use large macros (macro_rules!) that expand to the renderer and atlas implementations with blade namespace specified as argument.

Another solution would be to use generics, so structs of the same module can be instantiated with Vulkan/Metal or GLES backend specified as type parameter. However, @kvark does not want to offer traits over multiple backends, also for "code clarity" reasons.

Would you accept the large macro solution? Otherwise, you probably need to discuss this with @kvark.

@kvark
Copy link
Contributor

kvark commented Jul 23, 2024

To clarify, Blade does have traits - https://github.com/kvark/blade/blob/main/blade-graphics/src/traits.rs
They are just

  • non-comprehensive. The goal is to improve API consistency and don't accidentally miss something.
  • not exposed to the user. Nothing beats a straight object method call for API ergonomics.

It could be useful for this situation to at least expose the traits in public, maybe expand to cover a bit more functionality.

@maxdeviant
Copy link
Member

I’m going to close this until we decide it’s something we want to move forward with.

@maxdeviant maxdeviant closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants