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

Use std::vector instead of dynamic C-Array #6744

Merged
merged 1 commit into from Dec 10, 2017

Conversation

Projects
None yet
5 participants
@adrido
Copy link
Contributor

adrido commented Dec 5, 2017

Fixes #6740

I'm wondering, if it would possible/better to use a dynamic vector and floors.size() instead of num_floors and num_ceilings and a fixed vector.
But I does not want to break existing code and this PR is only meant to fix the failing build not for doing improvements.

@@ -194,7 +194,8 @@ class Mapgen {
s16 findLiquidSurface(v2s16 p2d, s16 ymin, s16 ymax);
void updateHeightmap(v3s16 nmin, v3s16 nmax);
void getSurfaces(v2s16 p2d, s16 ymin, s16 ymax,
s16 *floors, s16 *ceilings, u16 *num_floors, u16 *num_ceilings);
std::vector<s16> &floors, std::vector<s16> &ceilings,
u16 * num_floors, u16 * num_ceilings);

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Dec 5, 2017

Member

Pointer misplaced. u16 *num_floors, same for num_ceilings

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Dec 5, 2017

I suppose the C-style arrays were used for speed reasons, as the maximal size of the data amount is known. (ask @paramat ?)
You could still allocate the memory directly using s16 *floors = new s16[size] + delete[] floors or std::array<s16, size> floors.

@sfan5 sfan5 added the Maintenance label Dec 5, 2017

@paramat paramat added the @ Mapgen label Dec 6, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 6, 2017

Yes that's my code, i was unsure whether to use a vector instead of an array. I don't know of any advantage to an array, and vectors are used in most other places.
'floors' and 'ceilings' arrays are mostly used independently, one or the other, rarely both at the same time.

Will test.

@paramat paramat added the Windows label Dec 6, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Dec 6, 2017

I suppose the C-style arrays were used for speed reasons, as the maximal size of the data amount is known. (ask @paramat ?)
You could still allocate the memory directly using s16 *floors = new s16[size] + delete[] floors or std::array<s16, size> floors.

Thanks, I know. Its just my suggestion to fix #6740 . I don't know whether this function is time critical or not. A vector is just a more secure variant (and not necessary slower at all). Feel free to fix #6740 with another way. Every variant would fine for me as long as it fixes #6740. The merge process might be faster if a coredev is doing this.

@adrido adrido force-pushed the adrido:patch2 branch from b4ca0d9 to 22e4906 Dec 6, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 6, 2017

I've seen vector used in many places in mapgen so i doubt it's slower, or significantly slower. I probably should have used vector in my PR anyway.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 6, 2017

I'm no expert concerning the relative advantages of one against the other.

@paramat paramat added the One approval label Dec 6, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Dec 6, 2017

Thanks. A probably interesting article of C-Array vs std::array vs std::vector might be this by Bjarne Stroustrup: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon1-prefer-using-stl-array-or-vector-instead-of-a-c-array

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 6, 2017

Good, vector is fine for me.

@adrido adrido force-pushed the adrido:patch2 branch from 22e4906 to 7433dd8 Dec 6, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Dec 6, 2017

@paramat I added some improvements that got possible due to the usage of vectors. Could you please test it again?

@paramat paramat removed the One approval label Dec 6, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 6, 2017

Interesting, i'll rereview to understand and then retest. Removed my +1 for now.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 9, 2017

Ceiling decorations are not working, see line comment. My fix works.
👍 once fixed.

} else if (!is_walkable && walkable_above) {
ceilings[ceiling_i] = y + 1;
ceiling_i++;
ceilings.push_back(y);

This comment has been minimized.

Copy link
@paramat

paramat Dec 9, 2017

Member

ceilings.push_back(y + 1);

@adrido adrido force-pushed the adrido:patch2 branch from 7433dd8 to 0a0f159 Dec 9, 2017

@nerzhul
Copy link
Member

nerzhul left a comment

it's slower than C style array or std::array if no resize is done before passing them, please resize before sending the vectors
Don't forget std::vector push_back (or emplace_back) triggers a memory copy if it's full because std::vector is memory adjacent

@adrido adrido force-pushed the adrido:patch2 branch from 0a0f159 to f33b168 Dec 9, 2017

@adrido

This comment has been minimized.

Copy link
Contributor Author

adrido commented Dec 9, 2017

Thanks for reviewing. Done.
I just want to note:

  1. ) std::array will not work here too, because size is still not const (at compile time). GCC or Clang may support it, but its not official standard of C++

  2. )
    Stroustrup:

    People sometimes worry about the cost of std::vector growing incrementally. I used to worry about that and used reserve() to optimize the growth. After measuring my code and repeatedly having trouble finding the performance benefits of reserve() in real programs, I stopped using it except where it is needed to avoid iterator invalidation (a rare case in my code). Again: measure before you optimize.

    http://www.stroustrup.com/bs_faq2.html#slow-containers

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 10, 2017

Will retest then merge.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 10, 2017

Tested and fine 👍

@nerzhul nerzhul merged commit d677f29 into minetest:master Dec 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adrido adrido deleted the adrido:patch2 branch Dec 10, 2017

t0ny2 pushed a commit to t0ny2/minetest that referenced this pull request Jan 23, 2018

osjc added a commit to osjc/minetest that referenced this pull request Jan 11, 2019

osjc added a commit to osjc/minetest that referenced this pull request Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.