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.

Sign up for GitHub

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

Jump to bottom

Add range iterator to LocalVector #70773

Merged
merged 2 commits into from
Jan 21, 2023
Merged

Add range iterator to LocalVector #70773

merged 2 commits into from
Jan 21, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Dec 31, 2022

In this edition of enormous PR that touches every area of the engine I'm adding a range iterator to LocalVector type. I went ahead and also replaced all old loops where I found it applicable.

@KoBeWi KoBeWi added this to the 4.x milestone Dec 31, 2022
@KoBeWi KoBeWi requested review from a team as code owners December 31, 2022 15:57
@KoBeWi KoBeWi force-pushed the lector branch 5 times, most recently from bd43fb2 to dd3d53a Compare December 31, 2022 17:23
@Malcolmnixon Malcolmnixon mentioned this pull request Jan 3, 2023
Merged
akien-mga
akien-mga reviewed Jan 9, 2023
View reviewed changes
core/math/geometry_3d.cpp
int idx = faces[i].indices[j];
if (!vtx_remap.has(idx)) {
for (MeshData::Face &face : faces) {
for (int &index : face.indices) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it makes much sense to take the int as a reference.

akien-mga
akien-mga approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

That looks really good! I haven't reviewed in depth but we could merge after beta11 is released to have a week to catch potential regressions.

@lmurray
Copy link

lmurray commented Jan 13, 2023

A quick drive-by review: For a vector-like class it is possible to just return a raw pointer for begin() and end() without the need of an actual iterator class. This is how the STL usually implements the iterator for std::vector. As it stands the current iterator classes are just a passthrough and might get in the way of the optimizer.

@KoBeWi KoBeWi force-pushed the lector branch 3 times, most recently from a1081ba to e606267 Compare January 15, 2023 02:18
@akien-mga akien-mga modified the milestones: 4.x, 4.0 Jan 21, 2023
@akien-mga akien-mga merged commit c3539b4 into godotengine:master Jan 21, 2023
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the lector branch January 21, 2023 20:01
@bitsawer bitsawer mentioned this pull request Jan 25, 2023
Merged
@akien-mga akien-mga mentioned this pull request Jan 30, 2023
Merged
@RedworkDE RedworkDE mentioned this pull request May 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reduz reduz reduz approved these changes

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@KoBeWi @lmurray @akien-mga @reduz