Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement renderer resizing #402
Conversation
Xaeroxe
added
project: rendering
status: ready
type: feature
labels
Oct 11, 2017
Xaeroxe
requested review from
Rhuagh and
omni-viral
Oct 11, 2017
| + pub h: f32, | ||
| + /// Width divided by height. | ||
| + pub aspect_ratio: f32, | ||
| +} |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Rhuagh
Oct 11, 2017
Member
A dirty flag maybe? So the user doesn't have to cache the size for change detection. The RenderSystem can clear the dirty flag just before doing computation.
Rhuagh
Oct 11, 2017
Member
A dirty flag maybe? So the user doesn't have to cache the size for change detection. The RenderSystem can clear the dirty flag just before doing computation.
jojolepro
requested changes
Oct 11, 2017
Out of time to continue reviewing, will check back later.
| + pub fn new_target(&mut self, target: &Target) { | ||
| + for mut effect in &mut self.effects { | ||
| + effect.data.out_colors.clear(); | ||
| + effect |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jojolepro
Oct 11, 2017
Collaborator
Couln't you just clone the other color_bufs directly into the effect one, instead of iterating through? (Not sure it would clone the inner elements, though)
jojolepro
Oct 11, 2017
Collaborator
Couln't you just clone the other color_bufs directly into the effect one, instead of iterating through? (Not sure it would clone the inner elements, though)
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 11, 2017
Member
So yes, but it does come at the cost of an additional allocation, which I don't think is worth it.
Xaeroxe
Oct 11, 2017
Member
So yes, but it does come at the cost of an additional allocation, which I don't think is worth it.
| @@ -190,6 +209,18 @@ where | ||
| use std::cmp; | ||
| cmp::max(L::len() * jobs_count, 1) | ||
| } | ||
| + | ||
| + fn new_targets(&mut self, new_targets: &HashMap<String, Target>) { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| @@ -102,6 +104,15 @@ impl Renderer { | ||
| #[cfg(feature = "opengl")] | ||
| use glutin::GlContext; | ||
| + let size = self.window() | ||
| + .get_inner_size_pixels() | ||
| + .expect("Unable to fetch window size, as the window went away!"); |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jojolepro
Oct 11, 2017
Collaborator
Funny message, but if you could add "(window context lost, was the window closed?)" after, it might help the game devs figure out what happened more easily.
jojolepro
Oct 11, 2017
Collaborator
Funny message, but if you could add "(window context lost, was the window closed?)" after, it might help the game devs figure out what happened more easily.
| + | ||
| +/// Creates the Direct3D 11 backend. | ||
| +#[cfg(all(feature = "d3d11", target_os = "windows"))] | ||
| +fn init_existing_backend(window: &Window) -> Result<(Device, Factory, Target)> { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
omni-viral
Oct 11, 2017
Member
Is this correct way to recreate render target?
For example there is a function for openGL and similar one for DX
omni-viral
Oct 11, 2017
Member
Is this correct way to recreate render target?
For example there is a function for openGL and similar one for DX
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 11, 2017
Member
Hmm so it appears that only works for targets with a DepthStencilBuffer. Should we make a DepthStencilBuffer required?
Xaeroxe
Oct 11, 2017
Member
Hmm so it appears that only works for targets with a DepthStencilBuffer. Should we make a DepthStencilBuffer required?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
omni-viral
Oct 12, 2017
Member
You use those fuctions only for main target. And right now it has both unconditionally
omni-viral
Oct 12, 2017
Member
You use those fuctions only for main target. And right now it has both unconditionally
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Rhuagh
Oct 11, 2017
Member
Hmm, I was thinking the dirty flag for the user to be able to react to size changes, not for actually resizing the window. Does that even work?
|
Hmm, I was thinking the dirty flag for the user to be able to react to size changes, not for actually resizing the window. Does that even work? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 11, 2017
Member
oh ha! Yeah it does work, this code doesn't care how the window was resized, it just reacts to it being resized. I'm not sure how I feel about a single dirty flag being shared amongst all downstream code, I'd much rather they just check with cached size if that's what they're trying to do. It's a two word comparison vs a one word comparison, I doubt there will be severe performance issues.
|
oh ha! Yeah it does work, this code doesn't care how the window was resized, it just reacts to it being resized. I'm not sure how I feel about a single dirty flag being shared amongst all downstream code, I'd much rather they just check with cached size if that's what they're trying to do. It's a two word comparison vs a one word comparison, I doubt there will be severe performance issues. |
Xaeroxe
requested review from
omni-viral and
torkleyy
Oct 12, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
r? @jojolepro |
jojolepro
requested changes
Oct 12, 2017
•
Test results:
Ubuntu/Linux 17.04 Nividia
Pass
Windows 7 x64
Nvidia
Pass
Intel HD integrated
Fail
Surprisingly, resizing worked fine, for the first 5 seconds. When I started stretching the window really fast, I got it to crash with the error message:
thread 'main' panicked at 'called Result::unwrap() on an Err value: IoError(
Error { repr: Os { code: 0, message: "The operation completed successfully." } }
)', src\libcore\result.rs:860:4
note: Run with RUST_BACKTRACE=1 for a backtrace.
error: process didn't exit successfully: target\debug\examples\pong.exe (exit
code: 101)
If I'm the only one to get this error (which is not the same as last time we did try to resize and the same driver was crashing), then I'll approve and assume my computer is just broken
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 12, 2017
Member
@jojolepro So while I don't know what caused your error (which by the way is my personal favorite windows error message) I did add a bit more graceful handling for it in my latest commit.
|
@jojolepro So while I don't know what caused your error (which by the way is my personal favorite windows error message) I did add a bit more graceful handling for it in my latest commit. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 12, 2017
Member
So I loaded up my Windows 10 install with Intel HD integrated graphics and rapidly moved the right edge of the screen back and forth causing a large volume of resizing, annnnd I couldn't reproduce it :/
|
So I loaded up my Windows 10 install with Intel HD integrated graphics and rapidly moved the right edge of the screen back and forth causing a large volume of resizing, annnnd I couldn't reproduce it :/ |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
omni-viral
Oct 12, 2017
Member
IoError(
Error { repr: Os { code: 0, message: "The operation completed successfully." } }
)
Something is really wrong with fetching error from windows. As it calls GetLastError and there is none. So I presume it was cleared between failure and fetching. Or there was no error in the first place.
Can you @jojolepro show as the backtrace?
Something is really wrong with fetching error from windows. As it calls |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jojolepro
Oct 12, 2017
Collaborator
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IoError(
Error { repr: Os { code: 0, message: "The operation completed successfully." } }
)', src\libcore\result.rs:860:4
stack backtrace:
0: std::sys_common::backtrace::_print
at C:\projects\rust\src\libstd\sys_common\backtrace.rs:94
1: std::panicking::default_hook::{{closure}}
at C:\projects\rust\src\libstd\panicking.rs:379
2: std::panicking::default_hook
at C:\projects\rust\src\libstd\panicking.rs:396
3: std::panicking::rust_panic_with_hook
at C:\projects\rust\src\libstd\panicking.rs:611
4: std::panicking::begin_panic_new<alloc::string::String>
at C:\projects\rust\src\libstd\panicking.rs:553
5: std::panicking::begin_panic_fmt
at C:\projects\rust\src\libstd\panicking.rs:521
6: std::panicking::rust_begin_panic
at C:\projects\rust\src\libstd\panicking.rs:497
7: core::panicking::panic_fmt
at C:\projects\rust\src\libcore\panicking.rs:92
8: core::result::unwrap_failed<glutin::ContextError>
at C:\projects\rust\src\libcore\macros.rs:41
9: core::result::Result<(), glutin::ContextError>::unwrap<(),glutin::ContextE
rror>
at C:\projects\rust\src\libcore\result.rs:738
10: gfx_window_glutin::init_existing_raw
at C:\Users\jojolepro\.cargo\registry\src\github.com-1ecc6299db9ec8
23\gfx_window_glutin-0.17.0\src\lib.rs:140
11: gfx_window_glutin::init_existing<(gfx_core::format::R8_G8_B8_A8, gfx_core:
:format::Unorm),(gfx_core::format::D24_S8, gfx_core::format::Unorm)>
at C:\Users\jojolepro\.cargo\registry\src\github.com-1ecc6299db9ec8
23\gfx_window_glutin-0.17.0\src\lib.rs:94
12: amethyst_renderer::renderer::init_existing_backend
at .\amethyst_renderer\src\renderer.rs:390
13: amethyst_renderer::renderer::Renderer::resize<amethyst_renderer::pipe::pip
e::Pipeline<hetseq::list::List<(amethyst_renderer::pipe::stage::Stage<hetseq::li
st::List<(amethyst_renderer::pipe::pass::CompiledPass<amethyst_renderer::pass::f
lat::DrawFlat<amethyst_renderer::vertex::PosNormTex, amethyst::ecs::rendering::c
omponents::mesh::MeshComponent, amethyst::ecs::rendering::components::material::
MaterialComponent, amethyst::ecs::transform::components::transform::Transform>>,
hetseq::list::List<()>)>>, hetseq::list::List<()>)>>>
at .\amethyst_renderer\src\renderer.rs:149
14: amethyst_renderer::renderer::Renderer::draw<amethyst_renderer::pipe::pipe:
:Pipeline<hetseq::list::List<(amethyst_renderer::pipe::stage::Stage<hetseq::list
::List<(amethyst_renderer::pipe::pass::CompiledPass<amethyst_renderer::pass::fla
t::DrawFlat<amethyst_renderer::vertex::PosNormTex, amethyst::ecs::rendering::com
ponents::mesh::MeshComponent, amethyst::ecs::rendering::components::material::Ma
terialComponent, amethyst::ecs::transform::components::transform::Transform>>, h
etseq::list::List<()>)>>, hetseq::list::List<()>)>>>
at .\amethyst_renderer\src\renderer.rs:111
15: amethyst::ecs::rendering::systems::{{impl}}::run<amethyst_renderer::pipe::
pipe::Pipeline<hetseq::list::List<(amethyst_renderer::pipe::stage::Stage<hetseq:
:list::List<(amethyst_renderer::pipe::pass::CompiledPass<amethyst_renderer::pass
::flat::DrawFlat<amethyst_renderer::vertex::PosNormTex, amethyst::ecs::rendering
::components::mesh::MeshComponent, amethyst::ecs::rendering::components::materia
l::MaterialComponent, amethyst::ecs::transform::components::transform::Transform
>>, hetseq::list::List<()>)>>, hetseq::list::List<()>)>>>
at .\src\ecs\rendering\systems.rs:85
16: shred::system::{{impl}}::run_now<amethyst::ecs::rendering::systems::Render
System<amethyst_renderer::pipe::pipe::Pipeline<hetseq::list::List<(amethyst_rend
erer::pipe::stage::Stage<hetseq::list::List<(amethyst_renderer::pipe::pass::Comp
iledPass<amethyst_renderer::pass::flat::DrawFlat<amethyst_renderer::vertex::PosN
ormTex, amethyst::ecs::rendering::components::mesh::MeshComponent, amethyst::ecs
::rendering::components::material::MaterialComponent, amethyst::ecs::transform::
components::transform::Transform>>, hetseq::list::List<()>)>>, hetseq::list::Lis
t<()>)>>>>
at C:\Users\jojolepro\.cargo\registry\src\github.com-1ecc6299db9ec8
23\shred-0.5.0\src\system.rs:24
17: shred::dispatch::dispatcher::Dispatcher::dispatch_thread_local
at C:\Users\jojolepro\.cargo\registry\src\github.com-1ecc6299db9ec8
23\shred-0.5.0\src\dispatch\dispatcher.rs:80
18: shred::dispatch::dispatcher::Dispatcher::dispatch
at C:\Users\jojolepro\.cargo\registry\src\github.com-1ecc6299db9ec8
23\shred-0.5.0\src\dispatch\dispatcher.rs:38
19: amethyst::app::Application::advance_frame
at .\src\app.rs:194
20: amethyst::app::Application::run
at .\src\app.rs:122
21: pong::run
at .\examples\04_pong\main.rs:495
22: pong::main
at .\examples\04_pong\main.rs:499
23: panic_unwind::__rust_maybe_catch_panic
at C:\projects\rust\src\libpanic_unwind\lib.rs:98
24: std::rt::lang_start
at C:\projects\rust\src\libstd\rt.rs:52
25: main
26: __scrt_common_main_seh
at f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253
27: BaseThreadInitThunk
error: process didn't exit successfully: `target\debug\examples\pong.exe` (exit
code: 101)
It appears that my driver is for windows 8 while I'm on windows 7, don't ask me how that happened ;)
It appears that my driver is for windows 8 while I'm on windows 7, don't ask me how that happened ;) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Rhuagh
Oct 12, 2017
Member
So this is an unwrap deep inside glutin. Probably a race condition of some kind
|
So this is an unwrap deep inside glutin. Probably a race condition of some kind |
jojolepro
approved these changes
Oct 12, 2017
I'll assume my driver is still broken. Code looks good!
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 12, 2017
Member
Rebased, squashed, and implemented use of the function @omni-viral recommended.
Ready for review!
|
Rebased, squashed, and implemented use of the function @omni-viral recommended. Ready for review! |
| @@ -156,6 +156,17 @@ impl<P> CompiledPass<P> { | ||
| self.inner | ||
| .apply(Supplier::new(encoders, &mut self.effects[..]), data) | ||
| } | ||
| + | ||
| + pub fn new_target(&mut self, target: &Target) { | ||
| + for mut effect in &mut self.effects { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| + effect | ||
| + .data | ||
| + .out_colors | ||
| + .extend(target.color_bufs().iter().map(|cb| cb.as_output.clone())); |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| + .extend(target.color_bufs().iter().map(|cb| cb.as_output.clone())); | ||
| + effect.data.out_depth = target.depth_buf().map(|db| (db.as_output.clone(), (0, 0))); | ||
| + } | ||
| + } | ||
| } | ||
| impl<P> Debug for CompiledPass<P> { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| + let mut targets = HashMap::default(); | ||
| + targets.insert("".to_string(), self.main_target.clone()); | ||
| + for key in pipe.targets().keys() { | ||
| + if key != "" { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| + if key != "" { | ||
| + let (key, target) = TargetBuilder::new(key.clone()) | ||
| + .build(&mut self.factory, new_size) | ||
| + .unwrap(); |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 12, 2017
Member
If this was going to fail it would have failed when the game started and tried to create the target rather than later at the point of resizing.
Xaeroxe
Oct 12, 2017
Member
If this was going to fail it would have failed when the game started and tried to create the target rather than later at the point of resizing.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| @@ -167,3 +167,50 @@ impl WindowMessages { | ||
| self.queue.push(Box::new(command)); | ||
| } | ||
| } | ||
| + | ||
| +/// World resource that stores screen dimensions. | ||
| +pub struct ScreenDimensions { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| + fn run( | ||
| + &mut self, | ||
| + ( | ||
| + factory, |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 12, 2017
Member
Probably not, but rustfmt-nightly also seems to refuse to touch it, I'm not quite sure why.
Xaeroxe
Oct 12, 2017
Member
Probably not, but rustfmt-nightly also seems to refuse to touch it, I'm not quite sure why.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Fixed, r? @torkleyy |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Looks good! |
torkleyy
approved these changes
Oct 12, 2017
•
Looks good, but didn't check performance-wise. Especially the usage of HashMap concerns me, not sure if that can be avoided though.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Xaeroxe
Oct 12, 2017
Member
@torkleyy It only uses HashMap because that's what the renderer was using in the first place.
bors r+
|
@torkleyy It only uses HashMap because that's what the renderer was using in the first place. bors r+ |
Xaeroxe commentedOct 11, 2017
This pull request implements renderer resizing and also brings the unused and forgotten ScreenDimensions structure back into the fold, and adds it as a resource and keeps that resource updated.
cc @jojolepro Thanks for your prior work on this, it was helpful to be able to review your code while creating this.