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

[3.x] SurfaceTool - efficiency improvements #69723

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

lawnjelly
Copy link
Member

Changed to use LocalVector rather than linked lists.

This is actually part of #61568 but has been separated for easier review.

Notes

  • This is much faster than the previous code (and speed is critical for merging).
  • Conversion of SurfaceTool to use vectors rather than linked lists has already been done in master.

Areas of interest

  • Vertex now has fixed arrays for bones and weights, this allows more efficient access / movement within LocalVectors, and we don't support more than 4 bones in 3.x anyway.
  • Smoothing groups implementation is slightly altered but the end result should be the same.

scene/resources/surface_tool.cpp Outdated Show resolved Hide resolved
scene/resources/surface_tool.cpp Outdated Show resolved Hide resolved
scene/resources/surface_tool.cpp Outdated Show resolved Hide resolved
@lawnjelly lawnjelly force-pushed the surface_tool_revamp branch 2 times, most recently from 81219f2 to eeb787a Compare December 8, 2022 08:47
@pseidemann
Copy link

hi,
nice work!

just wanted to tell you there is an open pr #68034 (for 4.0) which changes the implementation of the smooth groups. not sure if it's relevant here

@lawnjelly
Copy link
Member Author

just wanted to tell you there is an open pr #68034 (for 4.0) which changes the implementation of the smooth groups. not sure if it's relevant here

Although the PR you linked looked sensible, I'm not attempting to make any changes to smoothing behaviour in this PR. Any changes should be in a separate PR considered on their own merit. This makes things easier to review. 👍

@clayjohn
Copy link
Member

clayjohn commented Dec 8, 2022

hi, nice work!

just wanted to tell you there is an open pr #68034 (for 4.0) which changes the implementation of the smooth groups. not sure if it's relevant here

In addition to lawnjelly's comment, note that this PR is targetting 3.x, so that PR is totally irrelevant as 3.x and 4.0 work differently with respect to smooth groups

@pseidemann
Copy link

my small hope was that maybe it's possible to consolidate some backporting work with this or to stay more compatible to 4.0 (or the other way around) to make it easier to backport things later

@lawnjelly
Copy link
Member Author

my small hope was that maybe it's possible to consolidate some backporting work with this or to stay more compatible to 4.0 (or the other way around) to make it easier to backport things later

Any changes to 3.x has to be backward compatible, so we can't easily change the existing behaviour. Nevertheless if there was a largely improved smoothing group implementation we could probably add it in parallel (as e.g. a new method) if there was demand and it had proved successful in 4.x.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Overall the change looks fine to me. I left a few style nitpicks, but the implementation looks good.

scene/resources/surface_tool.cpp Outdated Show resolved Hide resolved
scene/resources/surface_tool.cpp Outdated Show resolved Hide resolved
scene/resources/surface_tool.cpp Outdated Show resolved Hide resolved
scene/resources/surface_tool.h Outdated Show resolved Hide resolved
Changed to use LocalVector rather than linked lists.
@fire
Copy link
Member

fire commented Jan 26, 2023

@clayjohn @lawnjelly Is this pr ready to go?

@lawnjelly
Copy link
Member Author

@clayjohn @lawnjelly Is this pr ready to go?

Yup, changes made. 👍

@fire fire requested a review from a team January 27, 2023 15:17
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Seems fine to me. I tested locally and couldn't detect any regressions.

I tested on a mesh made of 100,000 primitive points and this PR improved the speed of creating and committing the mesh noticeably (~15% based on rough measurements)

@akien-mga akien-mga merged commit e365674 into godotengine:3.x Apr 17, 2023
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the surface_tool_revamp branch April 17, 2023 15:48
@akien-mga akien-mga changed the title SurfaceTool - efficiency improvements [3.x] SurfaceTool - efficiency improvements Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants