Skip to content
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

ECS: re-implement mesh rendering in database entries #2140

Merged

Conversation

kzsh
Copy link

@kzsh kzsh commented Aug 17, 2024

In the first few commits, this was close to a copy/paste from MeshRenderSystem::render3D to GuiRotatingModelView::onDraw. If duplicating the rendering pipeline in these two places is sufficient, I'm happy to simplify this PR back down to that.

That said, the rest of the work is an attempt at re-using some of the code between render3D and onDraw without compromising the clarity of the render system to serve the one-off needs of onDraw. I will admit that the likelihood of render code changing often is pretty low, so maybe this kind of re-use isn't that critical here.

Aside: Again, I'm pretty new to C++, so I'm open to thoughtful feedback on language features I'm misusing, or weaknesses I might be inadvertently introducing, or patterns that make more sense than what I've put in.

I've opted to save the debug work for a separate PR to keep this one a bit smaller, but I did leave one piece from my exploration there in this series of commits 2d0405c. I think it'll be useful to have an entity in order to enumerate and render emitters.

kzsh added 11 commits August 15, 2024 22:59
- Total duplication of the code there.  Will extract soon, if appropriate
The same projection is happening already a few lines above
These functions are somewhat arbitrary, but might be a
good starting point for a final form.  Providing these
functions from rendering.h is still only temporary.
My reflexes were for optional arguments, but an overload seems more
correct.
This seems to give a better result for a large variety of different original
model scales.  E.g. average vertex from 'center' of Atlantis is 30-40 whereas
an MP52 is 1ish.
src/systems/rendering.cpp Outdated Show resolved Hide resolved
src/systems/rendering.h Outdated Show resolved Hide resolved
kzsh added 10 commits August 18, 2024 21:08
This isn't necessary to render the mesh, but it will be useful
when trying to look up beam emitter and engine emitters to
render in debug mode
When copying behavior from viewport3D we adopted some behavior
from it that doesn't seem necessary for this view
@kzsh kzsh marked this pull request as ready for review August 20, 2024 22:05
src/mesh.cpp Outdated
}


float sum_x = 0.f;
Copy link
Owner

Choose a reason for hiding this comment

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

maybe it would be possible to use glm::vec3 instead? Not sure if you can initialize a glm::vec3 from a float[3].

But then it would look something like:

glm::vec3 sum{};
for(auto vertex : vertices) sum += glm::vec3(vertex.position);
auto average = sum / float(vertices.size());

for(auto vertex : vertices) {
    float distance = glm::length(glm::vec3(vertex.position) - average);
    ...
}

Copy link
Owner

Choose a reason for hiding this comment

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

(changing the Mesh float[3] into a glm::vec3 in theory should be possible, but right now feels quite risky and too large of a change)

Copy link
Author

Choose a reason for hiding this comment

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

I like that idea! That's where I started before changing it to try to match the style of the code above it, but I think it would be cleaner with glm structures/functions.

@daid daid merged commit fa060c9 into daid:ECS Aug 22, 2024
2 of 7 checks passed
@kzsh kzsh deleted the ECS_re-implement-mesh-rendering-in-database-entries branch August 28, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants