[WIP] update naga to 0.13.0 #37
Conversation
VitalyAnkh
force-pushed
the
update-naga-to-0.13
branch
from
6d32366
to
0152fd0
Compare
src/derive.rs
Outdated
| init: if c.r#override == Override::None { | ||
| self.import_const_expression(&c.init) | ||
| } else { | ||
| // r#override doesn't do anything now |
There was a problem hiding this comment.
it looks to me like init and override can both be specified - init is the default if an override is not provided on the pipeline.
There was a problem hiding this comment.
Yes, but the override doesn't do anything at this time. Maybe we should wait for naga to add functionality for this override field?
There was a problem hiding this comment.
i'd just import/copy both the fields over. no reason not to
src/derive.rs
Outdated
| init: self.import_const_expression(&c.init), | ||
| } | ||
| } | ||
| // TODO |
There was a problem hiding this comment.
const_expressions can be more than just constants, they can be compound expressions as long as all the components are const_expressions as well.
i haven't looked in a lot of detail, but i think rather than making a new function here, the easier thing to do is to use the existing import_expression fn passing self.const_expressions and self.shader.const_expressions in as the new and old arenas (instead of the function-local arenas that are used when import_expression is called in function import).
you will probably need to also make a member for the already_imported param, that gets reset when the shader is changed. this might mean we end up with duplicate const_expressions if multiple modules define the same constant, but that should be ok.
There was a problem hiding this comment.
use the existing import_expression fn passing self.const_expressions and self.shader.const_expressions in as the new and old arenas
With this approach I'm struggling with the borrow checker 🤕 🤕 🤕 The import_expression both borrows self.const_expressions and self.shader.const_expressions exclusively. Please give me some time to fix this.
There was a problem hiding this comment.
i think it would be pretty reasonable to change the already_imported and new_expressions params to RefCells (as well as the const_expressions and const_expressions_map members).
There was a problem hiding this comment.
I changed them to RefCells but there are many runtime BorrowMut errors when running tests, still working at it.
There was a problem hiding this comment.
i don't think you should change everything to refcell, just the const_expression bits (and the params for import_expression). i should have time at the weekend for a proper look if you get stuck.
There was a problem hiding this comment.
If I didn't change everything to RefCell, I can't change the &mut self to &self, which occurs in import_block, import_const and other import functions, then borrow checkings errors happens: self.import_expression(...&self.const_expression_map, &self.const_expressions,...) 🤕
i should have time at the weekend for a proper look if you get stuck.
Appreciate it! I'm going on a trip tomorrow so maybe I don't have much time at the weekend, but I will try my best to work with you and make this update happen.
|
closing in favour of #40. thanks for making a good start ! |
Still WIP. Fix #33.