-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Replace point constants with static class members #77730
base: master
Are you sure you want to change the base?
Conversation
Adding these to POINTS_COORDINATES.md wouldn't hurt |
f0022f7
to
e508928
Compare
e508928
to
589b9fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the changes are trivial and easily verified I feel like it should be okay even as a massive line diff.
src/activity_actor.cpp
Outdated
@@ -6570,8 +6569,8 @@ void chop_tree_activity_actor::finish( player_activity &act, Character &who ) | |||
const point_rel_ms main_dir = pos.xy() - who.pos_bub().xy(); | |||
const int circle_size = 8; | |||
static constexpr std::array<point, circle_size> circle = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be implementing point==tripoint
and using the existing eight_horizontal_neighbors
instead of defining a new static array, but I'm not going to throw a functional change like that into a PR that's already this big. I'll add const
here and come back to removing this on the next pass.
This looks like a good change. There was actually a sort of intention behind the set of constants that are defined and the ones that weren't, and that was that only it only made sense to have offsets for the relative types, with zero being used fairly frequently for absolute ones as well. However, it's only mildly detrimental to define everything for all types (detrimental, because it makes it easier to screw up and use an absolute offset constant when you really should have used a relative type, and similar errors). |
589b9fe
to
85d7a8b
Compare
85d7a8b
to
12140a0
Compare
Summary
None
Purpose of change
Currently whenever someone needs one of the point constants (zero, min, max, invalid, north, north_east, etc) for a coordinate type that hasn't been needed before, they add it to
coordinate_constants.h
. This file has accumulated over 100 such constants, in a hodge podge with no clear pattern to which constants exist and which constants don't and when it's appropriate to add a new one. This can make the codebase confusing to someone who seestripoint_abs_omt_foo
and wonders whytripoint_rel_ms_foo
doesn't exist.Describe the solution
I have moved the constants into static members of the
point
andtripoint
classes, then exposed them incoord_point_ob
. Now everytripoint_foo_bar
type has::zero
,::min
,::max
,::north
, etc.I also added a helper function to check whether a [tri]point equals the
::invalid
constant, which was the original goal of this effort and led me down the rabbit hole of makingpoint
andtripoint
behave more compatibly.I've attempted to update every usage, including various places that were using
_min
but should have been using_invalid
(which didn't exist in some cases) and are now using::invalid
.I described these constants in
POINTS_COORDINATES.md
.Describe alternatives you've considered
Exposing the constants on the coordinate types could be done in
coord_point_base
, but that felt too bloated.Testing
Automated tests pass. I am playing with this change and will un-draft this PR when I feel I've tested thoroughly.
Additional context
I'm not sure how best to break this up into multiple PRs. I could put the old constants back, change just a few files to the new constants, PR that, then continue until they are all changed?