Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upStore vertex attribs data in DOM and optimise GetVertexAttrib #21118
Conversation
highfive
commented
Jul 3, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jul 3, 2018
|
|
|
@bors-servo try |
Store vertex attribs data in DOM and optimise GetVertexAttrib This is not an extremely useful change on its own but those things need to be stored on the DOM side to implement some draw checks anyway. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21118) <!-- Reviewable:end -->
|
r? @emilio |
|
|
|
@bors-servo try |
Store vertex attribs data in DOM and optimise GetVertexAttrib This is not an extremely useful change on its own but those things need to be stored on the DOM side to implement some draw checks anyway. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21118) <!-- Reviewable:end -->
|
|
|
Looks great, sorry so much for the lag with this. |
| impl BoundAttribBuffers { | ||
| impl VertexAttribs { | ||
| pub fn new(max: u32) -> Self { | ||
| Self { attribs: DomRefCell::new(vec![Default::default(); max as usize].into()) } |
This comment has been minimized.
This comment has been minimized.
emilio
Jul 5, 2018
Member
How big can max_vertex_attribs be vs. what it's used? May it be worth it to grow it lazily or something?
| }) | ||
| fn delete_buffer(&self, buffer: &WebGLBuffer) { | ||
| for attrib in &mut **self.attribs.borrow_mut() { | ||
| if attrib.1.as_ref().map_or(false, |b| b.id() == buffer.id()) { |
This comment has been minimized.
This comment has been minimized.
| if attrib.1.as_ref().map_or(false, |b| b.id() == buffer.id()) { | ||
| attrib.1 = None; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn get(&self, index: u32) -> Option<Ref<WebGLBuffer>> { |
This comment has been minimized.
This comment has been minimized.
emilio
Jul 5, 2018
Member
A bunch of these should probably debug_assert!(index as usize < self.elements.borrow().len(), "Someone forgot to validate properly") or something of the sort, wdyt?
| pub struct BoundAttribBuffers { | ||
| elements: DomRefCell<FnvHashMap<u32, (bool, Option<Dom<WebGLBuffer>>)>>, | ||
| pub struct VertexAttribs { | ||
| attribs: DomRefCell<Box<[(bool, Option<Dom<WebGLBuffer>>)]>>, |
This comment has been minimized.
This comment has been minimized.
emilio
Jul 5, 2018
Member
It may be worth a comment here mentioning that the slice contains as many attribute buffers as possible, that the bool represents whether it's enabled as an array (should it be converted to a binary enum?), and that the buffer can be None when removed.
|
I added a comment about the small number of vertex attributes in high-end GPUs. @bors-servo r=emilio |
|
|
Store vertex attribs data in DOM and optimise GetVertexAttrib This is not an extremely useful change on its own but those things need to be stored on the DOM side to implement some draw checks anyway. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21118) <!-- Reviewable:end -->
|
|
|
@bors-servo retry #20734 |
|
|
|
|
nox commentedJul 3, 2018
•
edited by SimonSapin
This is not an extremely useful change on its own but those things need to be stored on the DOM side to implement some draw checks anyway.
This change is