ECS Refactor. #86
ECS Refactor. #86
Conversation
Safe interfaces of `AnyVec` removed, as tested and usage constrained.
Inochi2D node types into Inox2D components, see comments in `node/components.rs`. `Puppet` as self-managed struct, reads INP file into a tree of nodes and a world of components to construct self. No `Copy/Clone` on `InoxNode`, components and `JsonValue`, cutting Aka.inp loading time to 1/3.
Storing absolute transforms is no longer RenderCtx's job. Put graphics buffer stuff to a seperate file.
Should be easy for incorporating future renderable node types.
Get required data from components instead. Changes to the renderer traits: - remove `render()` - `on_begin_mask()` -> `on_begin_masks()` - `set_mask_mode()` -> `on_begin_mask()` - `draw_mesh_self()` -> `draw_textured_mesh_content()` - `draw_part()` -> `draw_drawable()`
…e backend gets smart. For example reusing some results for a same node.
Enum `Deform` and `DeformSrc` to get prepared for Meshgroup implementation. One `DeformStack` per deform-able node for combining multiple deforms in a controlled way. Drawable related stuff, especially the drawable kind determination logic, moved to its own places from `render.rs`, as apparently other systems (parameters for example) may want to know is a node a drawable.
…oss frames. so that `InoxNode` stores constant initial values.
The same reasoning behind `ctx` structs: Any data not in a model file is additional context data for certain functionalities, which should be only initialized upon demand. End users only reset and set the values of params. When and how to actually apply params per frame is up to this library. Removed `physics_ctx` placeholder that is present on local but should not be published.
General pattern for a "system": Takes the form of an optional `-Ctx` struct attached to a puppet, which can be initialized if dependencies met. Interactions with the puppet data are done in methods of the struct, so a `None` system can do nothing. Current systems: Transform, Param, Render.
With parameters and ECS system in place. Changes to `trait InoxRenderer` during implementation: - `new()` can have custom params. - `on_begin_draw()` and `on_end_draw()` Make `vao` creation, binding and unbinding, and vbo attaching, more explicit. Move previous OpenGL `InoxRenderer` implementation into new framework. Remove duplicate shader bind and uniform uploads during iteration over masks for a node (do once inside `on_begin_masks()`).
Properly implement zsort inheritance (+). Correctly exclude composite children from `root_drawables_zsorted`. Turn off param application temporarily, so this commit works for `Aka.inp`.
…d bug. Currently, overflow will happen if value (0, 0) is applied on a param, which is clearly a bug. The actual fix is not in the scope of ECS refactor.
The philosophy of the ECS refactor naturally leads to this cleanup: In this refactor, spec-defined variables either goes into `InoxNode` or a component. If there are additional variables needed for the execution of a certain module, they are attached as additional components later. It is revealed that bob position of a pendulum is such a variable. After cleanup, all pendulum-like systems can provide methods for accessing and updating their bobs' position, implementing `trait Pendulum`. And then `trait SimplePhysicsCtx` will be auto implemented for all `impl Pendulum`, handling the mapping from transforms to simulation input, from simulation input to parameter values. More comments are added.
The implementation is inefficient (check changes in `puppet.rs`), yes, but clear, and the bottleneck is not here anyways. Cloning of `SimplePhysics` is required for working around ownership problems, until dark magic implemented on `World` enabling multiple mutable handles to different components.
There was a problem hiding this comment.
I think overall I like this change. There's a few things here and there that I commented on but otherwise it's nice. I'll want to do another refactor pass on this when it's finalized and merged to improve on the doc comments you provided, and then later see what I can do about this very hacky AnyVec implementation. I'm probably going to replace it with an actual page-allocated arena, if all our supported targets allow for it.
inox2d/src/node/drawables.rs
Outdated
| /// Tries to construct a renderable node data pack from the World of components: | ||
| /// - `None` if node not renderable. | ||
| /// - Panicks if component combinations invalid. | ||
| pub(crate) fn new(id: InoxNodeUuid, comps: &'comps World) -> Option<Self> { |
There was a problem hiding this comment.
I'm not sure if this is the best place for this function or if it's even the right approach. It doesn't feel right to have to look at the components of a node only for it to return an enum that we have to then match against instead of doing it more directly. I don't have a nicer solution right now though but I'm probably gonna think about refactoring this part in the future.
There was a problem hiding this comment.
Related to above. This "component combination to enum variants" mapping is for following the Inochi2D spec. There should be a clear way of detecting "official" nodes, and then executing them in the "official" way.
Instead of removing this, what should be done further is a way to inject a end-user-provided routine to handle custom nodes, and before we hand over a node to a custom routine, we still need a way to extract the non-custom part and do what inox2d is supposed to do, which is what we have here, this new().
|
What does "another refactor pass" mean 👀? IMO we really should be doing MeshGroups and other rendering backends and even game engine integrations afterwards. Btw, my |
And suppress variant unused warning... until Meshgroups.
Responding to Inochi2D#86 (comment) Rename `ParamMapMode` to `PhysicsParamMapMode` for this change for clarity.
In response to Inochi2D#86 (comment)
In response to Inochi2D#86 (comment) Plus more robust `upload_array_to_gl()`, with more annotations inside.
In response to Inochi2D#86 (comment) ...and why `Sized` is required in the first place?
In response to Inochi2D#86 (comment) The core crate does not need to know about the presence of stuff to do before and after `draw()`. This is up to renderer implementations.
…mbinations for a Drawable. In response to Inochi2D#86 (comment)
…urce a `DeformStack`. Thus accordingly, fewer `DeformStack` resetting and reading will happen. Optimization inspired by Inochi2D#86 (comment)
For future meshgroup implementation.
|
Looks good to me. Now we just need to revive inox-wgpu |
|
After discussing it on Discord, we decided it was best to get rid of the WGPU renderer for now. Neither of us can maintain it because we don't know WGPU at all, and once either of us learns it, we're gonna need to rewrite it from scratch anyway because of this refactor and possble future ones to come. |
Putting my plans and designs here later.