Refactor more constants in TrackPaint.h #21995
Conversation
Harry-Hopkinson
force-pushed
the
refactor-trackpaint-constants
branch
from
702ee6d
to
3364379
Compare
Harry-Hopkinson
force-pushed
the
refactor-trackpaint-constants
branch
from
3364379
to
9daf835
Compare
src/openrct2/ride/TrackPaint.h
Outdated
| extern const CoordsXY defaultRightQuarterTurn5TilesOffsets[4][5]; | ||
| extern const CoordsXYZ defaultRightQuarterTurn5TilesBoundOffsets[4][5]; | ||
| extern const CoordsXY defaultRightQuarterTurn5TilesBoundLengths[4][5]; | ||
| constexpr CoordsXY kDefaultRightQuarterTurn5TilesOffsets[4][5] = { |
There was a problem hiding this comment.
I think these shouldn't be directly in the header, as this would create a new instance every time the header is included. Rather, they should remain in TrackPaint.cpp and exposed through a getter function.
If you do not feel like introducing such a getter function, you can leave the extern definition in TrackPaint.h.
There was a problem hiding this comment.
Should I do the same for the others that I did in #21942 etc?
There was a problem hiding this comment.
I think that @Harry-Hopkinson did this because Matt requested making the segment declarations constexpr. I can imagine this will be confusing, as it now looks like two team members are asking opposite things.
Let’s ask @ZehMatt to weigh in.
There was a problem hiding this comment.
When to use getters instead of constexpr is situational. I think in this case it might be best to move them into a new header and have only cpp files include it, adding getters always adds the risk of adding overhead and I think this is one of those areas in the code where avoiding branching code is probably for the better.
There was a problem hiding this comment.
So do you mean they should stay as they are now (extern), or?
There was a problem hiding this comment.
A new header, and changing them to constexpr, you mean?
If many cpp files pull from that header, doesn’t that create copies? Or will the compiler handle it correctly?
There was a problem hiding this comment.
Yes, that would create new instances of the arrays. But it also allows the compiler to inline them, potentially unroll a few loops, etc. As Matt said, it's situational 😅
There was a problem hiding this comment.
I'm a bit puzzled to why this PR changes them to begin with, best to revert that and just stick to refactoring the names. This is beyond a style refactor when we start moving data structures around.
There was a problem hiding this comment.
Fair enough. I’ll probably touch them anyway in a future track refactor.
@Harry-Hopkinson Could you take out the constexpr changes and just leave in the name changes?
Rename more constants to follow the new notation in TrackPaint.h. Move
extern conststoconstexpr.