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

Fornjot application can crash, if specific graphics features are not supported on target #33

Closed
hannobraun opened this issue Jan 10, 2022 · 2 comments
Labels
good first issue Good for newcomers topic: display Displaying Fornjot models type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

As of this writing, the graphics code uses one feature that might not be available everywhere: wgpu::Features::POLYGON_MODE_LINE

The code just requests that feature. If that features isn't available on the target (where the host application is executed), the host application would either fail with an error message, or even panic. This is not optimal, especially since POLYGON_MODE_LINE is only used for an optional drawing mode (rendering the lines of the triangle mesh).

Although I haven't seen or heard of any problems regarding this, it would be better to handle these optional features in a more robust way:

  1. Check which of features we want to use are actually supported.
  2. Only request those features that are actually available. Store that information somewhere.
  3. Don't let the user enable anything that requires features which are not available.

Ideally, there would be a nice error message in the UI, to let the user know what the problem is. But for now, it would already be an improvement, if the host application didn't crash if such a feature wasn't available.

@hannobraun hannobraun added type: bug Something isn't working good first issue Good for newcomers topic: display Displaying Fornjot models labels Jan 10, 2022
@hannobraun
Copy link
Owner Author

hannobraun commented Jan 10, 2022

I've marked this as https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, since it only requires some knowledge of the wgpu API, and very little knowledge of Fornjot.

The code that requests the features lives in renderer.rs:

// Don't just blindly assume that we can request this
// feature. If it isn't available, that might cause a panic,
// or an error to be returned here.
//
// See this issue:
// https://github.com/hannobraun/fornjot/issues/33
features: wgpu::Features::POLYGON_MODE_LINE,

This wgpu method can probably be used to determine which features are actually available:
https://docs.rs/wgpu/0.12.0/wgpu/struct.Adapter.html#method.features

I assume that which features are available would be stored in a field of Renderer. Probably makes sense to create a separate features module under graphics, with a Features struct to encapsulate this (I'm just musing here; don't feel bound by my ideas, if you think something else makes more sense).

Here's some drawing code that requires this feature to exist:

if config.draw_mesh {
drawables.mesh.draw(
&mut encoder,
&color_view,
&self.depth_view,
&self.bind_group,
);
}
if config.draw_debug {
drawables.lines.draw(
&mut encoder,
&color_view,
&self.depth_view,
&self.bind_group,
);
}

This code (and potentially other code like it) would need to check the features field of Renderer to determine whether it should execute. Eventually, this information should also be fed outside the renderer, so the UI knows what's up and can display a proper error message, but that could be done as an additional improvement down the line.

@hannobraun hannobraun changed the title Host application can crash, if specific graphics features are not supported on target Fornjot application can crash, if specific graphics features are not supported on target May 9, 2022
@hannobraun
Copy link
Owner Author

This issue has been addressed in #902. Thanks again, @hekno25!

I'm closing this issue, as Fornjot can no longer crash for this reason. I've opened the more specific #907 as a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers topic: display Displaying Fornjot models type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant