-
Notifications
You must be signed in to change notification settings - Fork 7
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
Basic texture implementation pt2 #63
Conversation
webrender/src/device.rs
Outdated
} | ||
} | ||
|
||
fn bind_texture(&mut self, device: &Device<B, hal::Graphics>, id: &TextureId, sampler: &TextureFilter, binding: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use &str
instead of String
7fccd90
to
67e8615
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Left a few notes here
webrender/src/device.rs
Outdated
@@ -46,6 +46,12 @@ pub const TEXTURE_HEIGHT: usize = 8; | |||
pub const MAX_INSTANCE_COUNT: usize = 1024; | |||
|
|||
pub type TextureId = u32; | |||
pub type FBOId = u32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we can't keep using the old types here? we need to keep the changes to the minimum
webrender/src/device.rs
Outdated
]); | ||
self.bind_texture(device, &device.bound_textures[0], &device.bound_sampler[0], "Color0"); | ||
} | ||
if device.bound_textures[1] != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could just do this in a loop
&TextureFilter::Linear => &device.sampler_linear, | ||
&TextureFilter::Nearest=> &device.sampler_nearest, | ||
}; | ||
device.device.update_descriptor_sets::<Range<_>>(&[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the future, we should call this less (i.e. combine multiple updates)
@@ -1402,7 +1472,10 @@ pub struct Device<B: hal::Backend, C> { | |||
blend_color: ColorF, | |||
// device state | |||
images: FastHashMap<TextureId, Image<B>>, | |||
fbos: FastHashMap<FBOId, Framebuffer<B>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the future, we'll think about getting rid of those maps
webrender/src/device.rs
Outdated
let mut fbo_id = DEFAULT_DRAW_FBO + 1; | ||
for _ in 0..count { | ||
while self.fbos.contains_key(&fbo_id) || fboids.contains(&fbo_id) { | ||
fbo_id = rng.gen_range::<u32>(DEFAULT_DRAW_FBO + 1, u32::max_value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, that's tricky.
I think what we need is just a Vec<Option<B::something>>
addressable by index, and so this generation routine would just pick the first unused index there, or allocate a new one
webrender/src/device.rs
Outdated
@@ -2215,18 +2307,42 @@ impl<B: hal::Backend> Device<B, hal::Graphics> { | |||
texture.filter = filter; | |||
texture.layer_count = layer_count; | |||
texture.render_target = render_target; | |||
//println!("texture.width={:?}", texture.width); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend to use the log
macros. It's already hooked up, all you need is to use debug!
,warn!
,info!
macros and become familiar with how to see it (e.g. RUST_LOG=webrender=debug cargo run ...
)
.push( | ||
self.images | ||
.get_mut(&texture.id) | ||
.expect("Texture not found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just do self.images[&texture.id]
for short, since the error handling doesn't do anything fancy here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it mutable
, but HashMap
does not have IndexMut
.
webrender/src/renderer.rs
Outdated
@@ -3334,7 +3358,13 @@ impl Renderer { | |||
BlendMode::SubpixelConstantTextColor(color) => { | |||
self.device.set_blend_mode_subpixel_constant_text_color(color); | |||
let mut program = self.ps_text_run.get(glyph_format, transform_kind, &mut self.device).unwrap(); | |||
// TODO: bind textures: batch.key.textures | |||
for i in 0 .. batch.key.textures.colors.len() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for (i, texture) in batch.key.textures.colors.iter().enumerate() {...}
webrender/src/renderer.rs
Outdated
@@ -4066,7 +4121,7 @@ impl Renderer { | |||
alpha.max_size.height as f32, | |||
ORTHO_NEAR_PLANE, | |||
ORTHO_FAR_PLANE, | |||
); | |||
).post_scale(1.0, -1.0, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put a big fat comment here saying why the scaling is needed. Or better - modify the parameters to ortho
so that scaling is not needed :)
} | ||
program.bind_instances( | ||
&self.device.device, | ||
&instances.iter().map(|pi| pi.into()).collect::<Vec<PrimitiveInstance>>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have bind_instances
accepting an iterator to avoid collections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just fixed the indentation here, but sure. I made a note for myself (actually for @zakorgy :) ) to look into this.
lgtm, I will look into the iterator part. |
A followup pr of #58.
With this, we can now draw into textures and use them in a later render pass.