Skip to content

refactor: split material, shader and model in new components#136

Merged
Miou-zora merged 14 commits intomainfrom
shader-material-separate-components
Mar 25, 2025
Merged

refactor: split material, shader and model in new components#136
Miou-zora merged 14 commits intomainfrom
shader-material-separate-components

Conversation

@ripel2
Copy link
Contributor

@ripel2 ripel2 commented Mar 19, 2025 •
edited
Loading

(Not related to any issue)

I have created three new components "Material", "Shader", and "Model" to store the name of the material and shader used in an object, as well as the object name itself.

I did this because of many things:

  • The "Model" component makes no sense: in an ECS objects are built via the association of multiple components that each stores one piece of data. The Model component currently holds: the mesh, the shader, and the material.
  • New components for material and shaders will be used for other elements of the engine (like the Text)
  • The "Mesh" struct should not be an "util"

Notes:

  • Handling of the important data for shaders and materials are still done through the ShaderManager / MaterialCache. No overhead was added here
  • Mesh is now a component
  • The three new components Material, Shader, and Model can all be created with a string and are now pre hashed to reduce overhead
  • The Model component now only holds a name, the ID of the object. (it was modelName previously)

You can test on branch esq-pr-136 OpenGL-Example/esq-pr-136

@ripel2 ripel2 added the refactor Code Refactor label Mar 19, 2025
@ripel2 ripel2 requested a review from a team March 19, 2025 14:18
@ripel2 ripel2 self-assigned this Mar 19, 2025
@ripel2 ripel2 changed the title refactor: split material and shaders in new components refactor: split material, shader and model in new components Mar 23, 2025
@ripel2 ripel2 marked this pull request as ready for review March 23, 2025 14:55
Miou-zora
Miou-zora requested changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@Miou-zora Miou-zora left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In term of "technical refactor" it looks nicer than before but there is a new type of data imerged from this PR: the Ref or the Handle (I don't now currently which one is the best namming for it). So basically we have multiple kind of data struct that store a unique ID linked to a resource (Material, Model, Shader) so we could create a templated struct that would store those kind of identifiers.

So it would change the name for structs (Material -> MaterialRef or MaterialHandle, etc. And MaterialData -> Material)

@ripel2
Copy link
Contributor Author

ripel2 commented Mar 24, 2025 •
edited
Loading

In term of "technical refactor" it looks nicer than before but there is a new type of data imerged from this PR: the Ref or the Handle (I don't now currently which one is the best namming for it). So basically we have multiple type of data struct that store a unique ID linked to a resource (Material, Model, Shader) so we could create a templated struct that would store those kind of identifiers.

So it would change the name for structs (Material -> MaterialRef or MaterialHandle, etc. And MaterialData -> Material)

I like the idea but "refs" are usually associated with memory managment (for instance, JoltPhysics have their own implemtation of smart pointers that are named "[object]Ref" (see here)

so Handle is probably the right choice ?

@Miou-zora
Copy link
Contributor

Miou-zora commented Mar 24, 2025

I like the idea but "refs" are usually associated with memory managment (for instance, JoltPhysics have their own implemtation of smart pointers that are named "[object]Ref" (see here)

so Handle is probably the right choice ?

As we will use a generic data struct, we could use something like AssetHandle<T> and yes, "Ref" can refer to many kind of thing now (like cpp ref, jolt ref) so it would be too ambiguous to use it i guess

@ripel2 ripel2 requested review from a team and Miou-zora March 24, 2025 16:36
@ripel2
Copy link
Contributor Author

ripel2 commented Mar 24, 2025

I like the idea but "refs" are usually associated with memory managment (for instance, JoltPhysics have their own implemtation of smart pointers that are named "[object]Ref" (see here)
so Handle is probably the right choice ?

As we will use a generic data struct, we could use something like AssetHandle<T> and yes, "Ref" can refer to many kind of thing now (like cpp ref, jolt ref) so it would be too ambiguous to use it i guess

For this PR I didn't do a generic handle but rather three different components.
Using templates would be a great idea to be able to link handles and their AssetsManager together but this should be defined in a new issue. (it is not in scope of this PR)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 24, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Miou-zora Miou-zora merged commit edd6972 into main Mar 25, 2025
12 checks passed
@Miou-zora Miou-zora deleted the shader-material-separate-components branch March 25, 2025 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Miou-zora Miou-zora Miou-zora approved these changes

Assignees

@ripel2 ripel2

Labels

refactor Code Refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ripel2 @Miou-zora