Refactoring concerning (mostly) MapWidths and MapHeights #120
Conversation
Added some Vec2i operators to ease their manipulation Removed some zombie code
|
Thank you for the contribution! Amalgamating the Width/Height variables into "Size" ones is a good idea, but the pull request is really quite massive, which I think is not optimal for reviewing the code and merging it. It seems to me that it would be better to split the pull request in multiple parts to make integrating it easier. Given how central these things are to the engine, even a pull request only merging CUnitType's TileWidth/TileHeight into TileSize would already be sizable. Another point is that I think it would be better to maintain the current variable names, only changing "Width"/"Height" for "Size". So that MapWidths/MapHeights would become MapSizes, instead of LayersSizes. Doing also has the advantage of reducing the probability of errors being introduced. There also seem to be quite a few changes that aren't a straightforward change from a width/height variable to a size one, which isn't a problem in itself, but it makes it more important to review the changed code, and that is complicated by the pull request's size. |
|
Le 10 sept. 2017 à 18:32, Andrettin ***@***.***> a écrit :
Thank you for the contribution!
Amalgamating the Width/Height variables into "Size" ones is a good idea, but the pull request is really quite massive,
I agree. Usually I try hard to write small commits, but I was overwelmed by the dependencies here, and my instincts does not helped here.
which I think is not optimal for reviewing the code and merging it. It seems to me that it would be better to split the pull request in multiple parts to make integrating it easier.
It would be ok for me, but merging those variables have impact on many lines.
However, I think I can rework the patch by starting with the enhancements on the vec2t template...
This could make it easier to build atomic patches (since the enhancements on this allow to use operators with integers... unfortunately, I could not find a way to make it work with vec2t of different Ts... maybe I should put more efforts on this first, otherwise compilation errors will be very annoying).
Given how central these things are to the engine, even a pull request only merging CUnitType's TileWidth/TileHeight into TileSize would already be sizable.
Ok, I will do that.
Another point is that I think it would be better to maintain the current variable names, only changing "Width"/"Height" for "Size". So that MapWidths/MapHeights would become MapSizes, instead of LayersSizes.
On this point, my goal was to try to have variables with real semantic, inspired by the comments.
I thought it would be confusing to have both a vec2i MapSize and a std::vector<vec2i> MapSizes.
Anyway, changing that is actually a matter of seconds thanks to sed :)
There also seem to be quite a few changes that aren't a straightforward change from a width/height variable to a size one, which isn't a problem in itself, but it makes it more important to review the changed code, and that is complicated by the pull request's size.
I know this patch is hard to review.
I tried to limit myself, but yes, on such a big change it's hard to focus on one thing after another, especially when the next changes are obvious and will imply patching the same lines again and again.
I will try to rework the patch in more atomic ones.
I guess:
1: improvements for the vec2t class
2: layersSizes -> mapSizes, with as few changes as possible accross the code
...: same for other W/H-like variables
n: code refactoring made possible by changes 2 to n-1
n+1: integrate the unrelated "accidental" :) changes I put by instinct (Like the containers growing thing, which should go in a function anyway, strange it's absent from standard).
Or maybe I should change LayersSizes (renamed, of course) after the other merges: it seems to be the one with the most dependencies, so hacking others first might maje the patch easier to review ?
What would you prefer?
|
Yes, I agree. In particular being able to add/subtract integers from Vec2i objects is a useful change, as is being able to define a Vec2i with only one number, used for both variables. About the issues with getting it to work with different Ts, doing something like this: ...didn't work properly? Or did you change the format of the operator functions for another reason entirely?
Thank you :)
You're correct that it is confusing... Changing the variable names isn't in itself a problem, it was more that in the context of a large patch it made it harder to review. I agree that "Size" instead of "MapSize" for Map.Info is more descriptive (since as it is a part of map information already, the "Map" is already implied). Speaking of which, I should rename the variables referring to surface layers to have SurfaceLayer instead of just Layer, to differentiate them from map layers themselves.
That sounds like a good plan :) I think first changing other instances of width/height, and later doing so for MapWidths/MapHeights would make most sense, since as you said the latter has a lot of references. If it would make your work easier for the formulation of the smaller patches, I could delete the dead code refrering to MapWidth/MapHeight, since it makes searching for actual code that uses those more difficult (specially since there are so many instances of that), similar to how you did in the large patch itself. |
|
Yes, I agree. In particular being able to add/subtract integers from Vec2i objects is a useful change, as is being able to define a Vec2i with only one number, used for both variables.
About the issues with getting it to work with different Ts, doing something like this:
template <typename T>
inline const Vec2T<T> &operator += (Vec2T<T> &lhs, int rhs)
{
lhs.x += rhs;
lhs.y += rhs;
return lhs;
}
...didn't work properly? Or did you change the format of the operator functions for another reason entirely?
The reason to have both parameters templated (with different template types) was to be able to work with unsigned, shorts, longs or even chars without warning or casting. I hate warnings and imo casts help to make code hard to read (but are preferable to warnings, except for C-style ones which hide too many pitfalls).
On this point, my goal was to try to have variables with real semantic, inspired by the comments.
I thought it would be confusing to have both a vec2i MapSize and a std::vector MapSizes.
Anyway, changing that is actually a matter of seconds thanks to sed :)
You're correct that it is confusing... Changing the variable names isn't in itself a problem, it was more that in the context of a large patch it made it harder to review.
Which makes sense.
Speaking of which, I should rename the variables referring to surface layers to have SurfaceLayer instead of just Layer, to differentiate them from map layers themselves.
I must admit I do not really understand what is meant by layers in wyrmgus. I have not really dug that anyway.
What would you prefer?
That sounds like a good plan :) I think first changing other instances of width/height, and later doing so for MapWidths/MapHeights would make most sense, since as you said the latter has a lot of references.
Ok then
If it would make your work easier for the formulation of the smaller patches, I could delete the dead code
To be honest, removing dead code can only help, and not only me, but anyone that would try to hack around, including you: smaller functions means they'll fit inside a single screen, and git diff will make much more sense without it. Just my opinion thought, and it is your project, so I ll simply follow.
But don't do it, at least not for that: I ll simply rework that patch, zombies will just be burned in the process.
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Most of the changes here were made to merge MapWidths and MapHeights in a single vector.
To ease the change, I had to hack around and do the same thing to some other Width/Height members of other classes. There is still a lot of room for improvements, but this PR is huge enough for now I think.
I tried to double check that I didn't put any typo, but I can't be absolutely certain about that.
If there is any point I can improve, I'll do as much as I can. Next commits for refactoring should be a lot smaller, I didn't expect that there was so much dependencies on those members.
PS: I am not at home, this is why I can profit of some computer to send that patch. Hopefully it will help, especially considering it's size.