Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAuto instancing #150
Conversation
vitvakatu
requested review from
kvark and
alteous
Dec 9, 2017
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
e580f3c
to
a4f7ee1
Dec 9, 2017
This comment has been minimized.
This comment has been minimized.
|
Holy crap this is impressive! Totally worth the wait. :) |
This comment has been minimized.
This comment has been minimized.
I'd prefer the original method. Let's define the optimizations to be at the implementation discretion. I.e. user should still be able to change the material properties (as opposed to changing to a different material entirely) without the loss of optimization. |
kvark
reviewed
Dec 11, 2017
|
Amazing work, @vitvakatu ! Aside from the mentioned concerns, my biggest one is: can we get away without atomic IDs on geometry and material? Could you explain in detail on why those are needed? |
| in vec4 a_World0; | ||
| in vec4 a_World1; | ||
| in vec4 a_World2; | ||
| in vec4 a_World3; |
This comment has been minimized.
This comment has been minimized.
| #include <globals> | ||
|
|
||
| in vec4 a_Position; | ||
| in vec4 a_Normal; | ||
| in vec2 a_TexCoord; | ||
| out vec2 v_TexCoord; | ||
| out vec4 v_Color; | ||
|
|
||
| in vec4 a_World0; |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 11, 2017
Collaborator
how about adopting a different prefix for the instance variables? say, i_XXX.
This comment has been minimized.
This comment has been minimized.
vitvakatu
Dec 11, 2017
Author
Member
I'm not familiar with naming conventions in shaders so yes, will do
| 1.9, | ||
| //-2.2, //TODO when performance allows | ||
| ]; | ||
| const SPEEDS: [f32; 6] = [0.7, -1.0, 1.3, -1.6, 1.9, -2.2]; |
This comment has been minimized.
This comment has been minimized.
| &mut self, | ||
| template: &Mesh, | ||
| material: M, | ||
| ) -> Mesh { | ||
| let mut hub = self.hub.lock().unwrap(); | ||
| let instances = self.backend |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 11, 2017
Collaborator
might want to have a helper private method in the factory for this, given how many times it's repeated
| @@ -70,6 +73,7 @@ pub struct Geometry { | |||
| pub shapes: HashMap<String, Shape>, | |||
| /// Faces. | |||
| pub faces: Vec<[u32; 3]>, | |||
| #[doc(hidden)] pub __id: usize, | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
vitvakatu
Dec 11, 2017
Author
Member
Unfortunately :( You can't use nice .. Default::default() syntax on structs with private fields
| /// | ||
| /// It will improve performance in case of multiple `Mesh`es with the same | ||
| /// materials and geometries. | ||
| pub fn use_instancing( |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 11, 2017
Collaborator
I'd like us to not have this explicit switch, preferably. We can technically draw everything with instancing. Whatever happens to never be instanced will just have an instance of 1. Whatever does get instanced with instance_mesh should be automatically fast.
| @@ -434,19 +466,29 @@ impl Renderer { | |||
| let quad_buf = gl_factory.create_constant_buffer(1); | |||
| let light_buf = gl_factory.create_constant_buffer(MAX_LIGHTS); | |||
| let pbr_buf = gl_factory.create_constant_buffer(1); | |||
| let inst_buf = gl_factory | |||
| .create_buffer( | |||
| INSTANCE_COUNT, | |||
This comment has been minimized.
This comment has been minimized.
kvark
Dec 11, 2017
Collaborator
instead of keeping around a fixed-inst-count buffer, I think we should have an automatically grown instance buffer per shader. We collect CPU instances first, then see if the buffer needs to be re-allocated (and do this), then fill out instance data.
This comment has been minimized.
This comment has been minimized.
vitvakatu
Dec 11, 2017
Author
Member
If so, we need to store factory inside Renderer. Can't remember whether it's possible (probably yes)
| @@ -953,6 +939,109 @@ impl Renderer { | |||
| self.encoder.flush(&mut self.device); | |||
| } | |||
|
|
|||
| #[inline] | |||
| fn instance_basic( | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
About IDs: you need to group meshes with same geometry and material in renderer. But both geometry and material structs are unhashable (and you can't do them hashable without some unique field). Maybe there's another solution? |
This comment has been minimized.
This comment has been minimized.
|
Okay, all concerns from @kvark were covered. Awaiting further review |
kvark
reviewed
Dec 15, 2017
|
Neat! Looks like we are getting there :) |
| #include <globals> | ||
|
|
||
| in vec4 a_Position; | ||
| in vec4 a_Normal; | ||
| in vec2 a_TexCoord; | ||
| out vec2 v_TexCoord; | ||
| out vec4 v_Color; | ||
|
|
||
| in vec4 i_World0; |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 15, 2017
Collaborator
would it make sense to move those instance inputs into a header ("locals")? hint: it will not if different shaders have different instance inputs
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| in vec4 i_World0; | ||
| in vec4 i_World1; | ||
| in vec4 i_World2; | ||
| in vec4 i_World3; |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 15, 2017
Collaborator
as I mentioned before (and I don't think there was any reply?), we should use 3 vectors instead of 4 for the world transformation.
This comment has been minimized.
This comment has been minimized.
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct Material { | ||
| pub(crate) mat_type: MaterialType, | ||
| pub(crate) id: usize, |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
vitvakatu
Dec 16, 2017
Author
Member
No, we can't. Materials contains f32s, that are unhashable. Even if we can find a way around it, our Texture<[f32; 4]> is unhashable too because of f32 again.
This comment has been minimized.
This comment has been minimized.
kvark
Dec 16, 2017
Collaborator
f32 in general are unhashable, but only because of the exceptions like de-normalized floats and NaN, Inf, etc. We can safely assume the f32 in the material are not exceptional (and abort otherwise). Hashing can't be derived automatically, but it can be implemented using one of the following methods:
- just hashing raw 32bits of the float
- if the float is known to be in a rage (say, 0-1), quantitize it with enough precision (
u32is more than enough).
Texture<[f32; 4]> is just a handle, once again, that is hashable. Caveat: we need to investigate in depth the safety of hashing the gfx handles, but for now this is an acceptable choice.
| color: u32, | ||
| uv_range: [f32; 4], | ||
| param: f32, | ||
| ) -> Instance { |
This comment has been minimized.
This comment has been minimized.
| pub pending: Option<DynamicData>, | ||
| pub use_instancing: bool, | ||
| pub instance_cache_key: InstanceCacheKey, |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 15, 2017
Collaborator
could we have Option<InstanceCacheKey> for clearer semantics than a separate boolean flag?
|
|
||
| let slice = if instances.len() > 1 { | ||
| let mut slice = slice; | ||
| slice.instances = Some((instances.len() as u32, 0)); |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 15, 2017
Collaborator
oh my, so many slice things!
could do Slice { instances: <something>, ..slice }
| #[derive(Clone, Debug)] | ||
| pub(crate) struct PbrMapParams { | ||
| pub(crate) base_color: ( | ||
| h::ShaderResourceView<BackendResources, [f32; 4]>, |
This comment has been minimized.
This comment has been minimized.
kvark
Dec 15, 2017
Collaborator
let's have a type for shader view + sampler, given that we need it many times here
alteous
reviewed
Dec 19, 2017
| @@ -54,6 +62,8 @@ pub mod basic { | |||
| } | |||
|
|
|||
| /// Parameters for a Lamberian diffusion reflection model. | |||
| /// | |||
| /// Renders triangle meshes with the Gouraud illumination model. | |||
This comment has been minimized.
This comment has been minimized.
alteous
Dec 19, 2017
Member
*Lambertian illumination model.
Gouraud shading refers to whether lighting computed at the vertices of a polygon are interpolated across the face, hence the flat field disables/enables Gouraud shading.
This comment has been minimized.
This comment has been minimized.
alteous
Dec 19, 2017
•
Member
I considered once having an enum Shading { Flat, Gouraud } but that felt a bit overkill.
This comment has been minimized.
This comment has been minimized.
vitvakatu
Dec 20, 2017
Author
Member
Sure, I've been moving doc lines forth and back, so it's not the last change.
alteous
reviewed
Dec 19, 2017
| MaterialType::Lambert(_) => &self.mesh_gouraud, | ||
| MaterialType::Phong(_) => &self.mesh_phong, | ||
| MaterialType::Sprite(_) => &self.sprite, | ||
| _ => unreachable!(), |
This comment has been minimized.
This comment has been minimized.
alteous
Dec 19, 2017
Member
Is it really unreachable or just an internal error if we hit this branch?
This comment has been minimized.
This comment has been minimized.
vitvakatu
Dec 20, 2017
Author
Member
Unreachable for sure (if somebody won't break the renderer logic).
kvark
approved these changes
Dec 24, 2017
|
Looks good! Anything else baking in the oven? |
This comment has been minimized.
This comment has been minimized.
|
Yep, a few more things - safe wrappers around floats and grouping world matrix in 3 vectors. |
alteous
reviewed
Jan 1, 2018
| impl Hash for SafeFloat { | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| let mut buf = [0; 4]; | ||
| LittleEndian::write_f32(&mut buf, self.0); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
kvark
Jan 2, 2018
Collaborator
let's use an even better thing: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.to_bits
This comment has been minimized.
This comment has been minimized.
|
Still WIP? |
This comment has been minimized.
This comment has been minimized.
|
Still WIP :( |
vitvakatu
changed the title
[WIP] Auto instancing
Auto instancing
Jan 2, 2018
kvark
requested changes
Jan 2, 2018
| @@ -786,6 +786,7 @@ impl Renderer { | |||
| }; | |||
|
|
|||
| let mx_world: [[f32; 4]; 4] = Matrix4::from(node.world_transform).into(); | |||
| println!("{:?}", mx_world); | |||
This comment has been minimized.
This comment has been minimized.
| pub struct SafeFloat(f32); | ||
|
|
||
| impl SafeFloat { | ||
| pub fn try_from(value: f32) -> Option<Self> { |
This comment has been minimized.
This comment has been minimized.
| impl Hash for SafeFloat { | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| let mut buf = [0; 4]; | ||
| LittleEndian::write_f32(&mut buf, self.0); |
This comment has been minimized.
This comment has been minimized.
kvark
Jan 2, 2018
Collaborator
let's use an even better thing: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.to_bits
alteous
reviewed
Jan 2, 2018
| @@ -182,7 +212,45 @@ impl Default for Pbr { | |||
| } | |||
| } | |||
|
|
|||
| #[doc(hidden)] | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
alteous
Jan 2, 2018
Member
Hmm... I'm not happy about having two separate material kinds. Perhaps we should make Material opaque for now?
kvark
requested changes
Jan 2, 2018
| in vec4 i_Color; | ||
| in vec4 i_UvRange; | ||
|
|
||
| void main() { | ||
| mat4 m_World = mat4(i_World0, i_World1, i_World2, i_World3); | ||
| mat4 m_World = world_matrix(i_World0, i_World1, i_World2); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,7 @@ | |||
| mat4 world_matrix(vec4 i_World0, vec4 i_World1, vec4 i_World2) { | |||
This comment has been minimized.
This comment has been minimized.
kvark
Jan 2, 2018
Collaborator
I don't think it needs a header, tbh, since all you need is
transpose(mat4(i_World0, i_World1, i_World2, vec4(0.0,0.0,0.0,1.0)))
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
alteous
reviewed
Jan 2, 2018
| pub struct SafeFloat(f32); | ||
|
|
||
| impl SafeFloat { | ||
| pub fn try_from(value: f32) -> Option<Self> { |
This comment has been minimized.
This comment has been minimized.
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
77ce0e7
to
d6d0648
Jan 2, 2018
vitvakatu
added some commits
Nov 9, 2017
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
3 times, most recently
from
e341ddc
to
941c747
Jan 2, 2018
This comment has been minimized.
This comment has been minimized.
|
Updated to gfx 16.3, rebased, ready for merge |
This comment has been minimized.
This comment has been minimized.
|
Thanks! A few more things I spotted. Reviewed 6 of 17 files at r1, 4 of 10 files at r3, 7 of 9 files at r4, 7 of 8 files at r5. Cargo.toml, line 27 at r5 (raw file):
this is not needed examples/group.rs, line 135 at r5 (raw file):
plz make it prettier a bit, describe what the number is for src/material.rs, line 202 at r5 (raw file):
this goes against our "convention" (e.g. src/material.rs, line 265 at r5 (raw file):
same here, let's be consistent src/safe_float.rs, line 17 at r5 (raw file):
could be src/safe_float.rs, line 28 at r5 (raw file):
wonder if it's going to be simpler to store it as src/texture.rs, line 26 at r5 (raw file):
can this impl be auto-derived? src/factory/mod.rs, line 263 at r5 (raw file):
why is src/factory/mod.rs, line 305 at r5 (raw file):
can we do this in-place below, like the old code did? src/factory/mod.rs, line 368 at r5 (raw file):
does it need a src/factory/mod.rs, line 889 at r5 (raw file):
why is src/render/mod.rs, line 28 at r5 (raw file):
let's move it out of src/render/mod.rs, line 214 at r5 (raw file):
src/render/mod.rs, line 216 at r5 (raw file):
hmm, shouldn't it be transposed here? src/render/mod.rs, line 220 at r5 (raw file):
we could make this helper more useful by adding a parameter for src/render/mod.rs, line 229 at r5 (raw file):
Comments from Reviewable |
This comment has been minimized.
This comment has been minimized.
|
Review status: all files reviewed at latest revision, 16 unresolved discussions. Cargo.toml, line 27 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Oh, rebasing issue. Nicely spotted! src/safe_float.rs, line 17 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
No, Comments from Reviewable |
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
941c747
to
38c1b6d
Jan 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Review status: all files reviewed at latest revision, 16 unresolved discussions. Cargo.toml, line 27 at r5 (raw file): Previously, vitvakatu (Ilya Bogdanov) wrote…
Done. examples/group.rs, line 135 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/material.rs, line 202 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/material.rs, line 265 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/safe_float.rs, line 28 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
I prefer to leave it for now src/texture.rs, line 26 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
No, because of type parameter src/factory/mod.rs, line 263 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/factory/mod.rs, line 305 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/factory/mod.rs, line 368 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Yes src/factory/mod.rs, line 889 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/render/mod.rs, line 28 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/render/mod.rs, line 214 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/render/mod.rs, line 216 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
I assume that it's already transposed, so not src/render/mod.rs, line 220 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Will cover in follow-up src/render/mod.rs, line 229 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. Comments from Reviewable |
This comment has been minimized.
This comment has been minimized.
|
Reviewed 5 of 5 files at r6. src/texture.rs, line 26 at r5 (raw file): Previously, vitvakatu (Ilya Bogdanov) wrote…
Will be possible with src/render/mod.rs, line 216 at r5 (raw file): Previously, vitvakatu (Ilya Bogdanov) wrote…
I see. Let's either have it reflected in the parameter name, or even use the strongly typed src/render/mod.rs, line 220 at r5 (raw file): Previously, vitvakatu (Ilya Bogdanov) wrote…
thanks! please add a TODO comment Comments from Reviewable |
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
38c1b6d
to
a2df0d0
Jan 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Review status: all files reviewed at latest revision, 3 unresolved discussions. src/texture.rs, line 26 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/render/mod.rs, line 216 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Good advice, found issue in shadow rendering. Done. src/render/mod.rs, line 220 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. Comments from Reviewable |
This comment has been minimized.
This comment has been minimized.
|
Reviewed 2 of 2 files at r7. src/render/mod.rs, line 216 at r5 (raw file): Previously, vitvakatu (Ilya Bogdanov) wrote…
extra nit: no need to reflect it no both the name and the type. Only the type is clear enough, and we can have a shorter name ;) Comments from Reviewable |
kvark
approved these changes
Jan 3, 2018
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
a2df0d0
to
91d63f4
Jan 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Review status: all files reviewed at latest revision, 1 unresolved discussion. src/render/mod.rs, line 216 at r5 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. Comments from Reviewable |
This comment has been minimized.
This comment has been minimized.
|
Reviewed 1 of 1 files at r8. Comments from Reviewable |
vitvakatu
added some commits
Dec 23, 2017
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
91d63f4
to
6f2f20c
Jan 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Reviewed 7 of 7 files at r9. src/texture.rs, line 4 at r9 (raw file):
I typically order imports into groups: dependencies, this crate, std. Not a big deal though, I assume rustfmt will overwrite this? src/texture.rs, line 21 at r9 (raw file):
let's split in 2 lines, like everywhere else Comments from Reviewable |
vitvakatu
force-pushed the
vitvakatu:auto_instancing
branch
from
6f2f20c
to
d0f7539
Jan 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Review status: all files reviewed at latest revision, 2 unresolved discussions. src/texture.rs, line 4 at r9 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Done. src/texture.rs, line 21 at r9 (raw file): Previously, kvark (Dzmitry Malyshau) wrote…
Oh, it's rustfmt. Should I disable formatting for this struct? Comments from Reviewable |
This comment has been minimized.
This comment has been minimized.
|
|
bors bot
added a commit
that referenced
this pull request
Jan 3, 2018
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
Build succeeded |
vitvakatu commentedDec 9, 2017
•
edited
fixes #37
Q&A:
Insanity?
No, just auto instancing support. (I'm lying, it's crazy)
What have you done?
Short list:
Locals, just vertex attributes.Materialenum was renamed toMaterialType, made private, added newMaterialstruct.Why? Because I need UIDs to group meshes in renderer.
rendermodule in order to reduce code duplication for both instanced and regular rendering.mesh_instance*was renamed toduplicate_mesh*. Reason - you can't change material for mesh with instancing support. (Well, actually you can, but you will lose optimization. Perhaps I should keep this method?)Worth it?
Totally! In group example, performance have increased from 14 to stable 60 FPS (in release mode).
Some things looks really weird, so ask me please.
Renderer now looks like a total mess, but I would like to continue my work on it.