prevent crash when d3d9 render in background #192
Conversation
There was a problem hiding this comment.
Thank you for your contribution! Sorry for the monstruous delay but I just haven't had a lot of time to dedicate to this library lately.
The concept looks okay to me, the changes requested below are mostly stylistic. In addition to them, could you please add a Pipeline::reset method that just calls the same method on the engine? After that, feel free to mark this PR as non-draft and we can merge it in. Once that's done, I plan to do an immediate patch release.
Thank you!
| @@ -90,7 +91,17 @@ impl RenderContext for D3D9RenderEngine { | |||
|
|
|||
| impl RenderEngine for D3D9RenderEngine { | |||
| type RenderTarget = IDirect3DSurface9; | |||
|
|
|||
| fn reset(&mut self) { | |||
| tracing::error!("Calling reset stuff"); | |||
There was a problem hiding this comment.
I'd rather use tracing::trace! for this, as this is not an error.
To keep in line with the general crate style, could you please add a use tracing::{error, ...} statement at the top? Same for the instances below.
| let result = self.device.Reset(&mut params as *mut D3DPRESENT_PARAMETERS); | ||
| match result { | ||
| Ok(_) => tracing::info!("All is good"), | ||
| Err(err) => tracing::error!("Error from reset: {}", err), | ||
| } |
There was a problem hiding this comment.
I'd rather not report the success case at all, and only manage the error case instead. I'd also use the single e character name out of style consistency with the codebase.
| let result = self.device.Reset(&mut params as *mut D3DPRESENT_PARAMETERS); | |
| match result { | |
| Ok(_) => tracing::info!("All is good"), | |
| Err(err) => tracing::error!("Error from reset: {}", err), | |
| } | |
| if let Err(e) = self.device.Reset(&mut params as *mut D3DPRESENT_PARAMETERS) { | |
| Err(err) => tracing::error!("Error from reset: {}", err), | |
| } |
| @@ -79,6 +79,7 @@ impl RenderContext for D3D9RenderEngine { | |||
|
|
|||
| fn replace_texture( | |||
| &mut self, | |||
|
|
|||
| tracing::error!("WM_ACTIVATEAPP trigered, calling reset from engine"); | ||
| pipeline.engine.reset(); |
There was a problem hiding this comment.
Similar to above, I wouldn't report this event at all as it is explicitly necessary, and also I'd change this to be a method of pipeline instead, so we can avoid making the engine field pub:
| tracing::error!("WM_ACTIVATEAPP trigered, calling reset from engine"); | |
| pipeline.engine.reset(); | |
| pipeline.reset(); |
| @@ -41,7 +41,7 @@ pub(crate) struct PipelineSharedState { | |||
| pub(crate) struct Pipeline<T: RenderEngine> { | |||
| hwnd: HWND, | |||
| ctx: Context, | |||
| engine: T, | |||
| pub engine: T, | |||
There was a problem hiding this comment.
I'd rather not make struct members public unless there is an explicit need to do so:
| pub engine: T, | |
| engine: T, |
I opened the issue #191 because pressing keys ctrl+alt+supr or alt+tab made my D3D9 imgui menu to disapear.
After looking at this topic: 256699-alt-tab-support-directx.html, i noticed that the WM_ACTIVATEAPP was not handled. I made a little POC that call the Reset method on the device. My menu is still present after that, even if there is rendering errors in the console.
This PR should not be merged directly