Overmap point refactoring #41693

jbytheway · 2020-06-30T01:53:06Z

Summary

SUMMARY: Infrastructure "Make use of new point types for overmap-scale code"

Purpose of change

I've been working towards greater type-safety in our point types for quite some time. #32017 introduces the new types. However, to know whether they are fit for purpose requires testing them at scale in the codebase. That's what this PR does.

This PR is not really intended to be merged directly. I'm looking for feedback on the new API, as demonstrated by this PR. If it meets with approval, then I can start making more, smaller, PRs to convey these changes more digestibly (although there will still be one giant PR in there with most of these changes, it needn't be all of them).

Now I've pulled out all the easily separable pieces, this is now intended to be merged, and is still fairly sizable.

Describe the solution

Refactor overmap and overmapbuffer to use the new point types, and deal with all the consequent fallout.

Involves more changes than I originally anticipated, but serves as a good demonstration of the new point type features.

Fixed some bugs along the way that were discovered by this new point type safety. The ones I remember are:

The evacuation shelter now correctly calculates the distance and direction to the refugee center, previously it was mostly nonsense (this only affects the message it prints out).
Weather conditions for NPC-planted crops are calculated more reasonably. Previously they were passing an overmap location as a map-square location, which could have caused all sorts of strangeness.

Wherever these new point types interface with legacy point types I've added the comment // TODO: fix point types as a reminder that these new types should gradually spread over the rest of the codebase over time.

Describe alternatives you've considered

Finding a more reasonable-sized chunk of code to refactor.

Testing

Mostly "if it compiles it works". I tried to be low-risk in my changes wherever possible, and not change behaviour except when I was really sure it was a bug.

Additional context

This took far too long.

mlangsdorf

I reviewed faction camps, missions, and NPCs, and glanced over map, mapgen, and iexamine. My eyes are beginning to glaze over so I'll come back and review vehicles later.

Overall, this looks great! Clarity is much improved, and I think writing code in the future that deals with overmap scale tripoints should be much easier.

src/faction.cpp

mlangsdorf · 2020-06-30T10:51:03Z

src/faction_camp.cpp

-    tripoint forest = om_target_tile( omt_pos, 10, 90, hide_locations, true, true,
-                                      omt_pos, true );
-    if( forest != tripoint( -999, -999, -999 ) ) {
+    tripoint_abs_omt forest = om_target_tile( omt_pos, 10, 90, hide_locations, true, true,


Note to self: why isn't this a cata::optional? Oh, right, acidia and I never went back and cleaned up the stuff that mostly worked.

Yeah, I'd like to change that, but trying not to do unrelated refactoring in this commit.

mlangsdorf · 2020-06-30T10:54:23Z

src/faction_camp.cpp

-    tripoint mapmax = tripoint( 2 * SEEX - 1, 2 * SEEY - 1, omt_tgt.z );
+    farm_map.load( project_to<coords::scale::submap>( omt_tgt ), false );
+    tripoint mapmin = tripoint( 0, 0, omt_tgt.z() );
+    tripoint mapmax = tripoint( 2 * SEEX - 1, 2 * SEEY - 1, omt_tgt.z() );


Both of these expressions seem like they should be standard utility functions.

Agreed. But this is mapscale stuff and I was trying to limit these changes to overmap-scale stuff, lest this PR get even more ridiculously large.

src/iexamine.cpp

src/map.cpp

mlangsdorf · 2020-06-30T11:08:07Z

src/npc.cpp

+    for( int x2 = -3; x2 < 3; x2++ ) {
+        for( int y2 = -3; y2 < 3; y2++ ) {
+            const point_abs_omt nearby = p + point( x2, y2 );
+            cata::optional<basecamp *> bcp = overmap_buffer.find_camp( nearby );


Note to self: find_camp should take a search radius.

src/npc.cpp

src/npcmove.cpp

jbytheway · 2020-06-30T13:18:12Z

Thanks for the review @mlangsdorf. Sorry if it appears that I'm deflecting all of your comments because I'm not actually changing anything in response to them. Feel free to push back if my responses are unsatisfactory.

mlangsdorf

I expected there would be a lot more stuff in the vehicle code.

The PR looks good overall. It definitely needs follow-up on the map square and submap scale, and then a general refactor and cleanup of all the ugly code idioms that have snuck in over the years.

And don't worry about not changing anything in response to my questions - sometimes, I just want a justification for why you made a decision, which is why I ask so many questions. If there was something I really felt you needed to change, I would have been much more direct about it.

mlangsdorf · 2020-06-30T18:15:40Z

src/vehicle.cpp

-    point omt_diff = omt_path.back().xy() - veh_omt_pos.xy();
-    if( omt_diff.x > 3 || omt_diff.x < -3 || omt_diff.y > 3 || omt_diff.y < -3 ) {
+    point_rel_omt omt_diff = omt_path.back().xy() - veh_omt_pos.xy();
+    if( omt_diff.x() > 3 || omt_diff.x() < -3 || omt_diff.y() > 3 || omt_diff.y() < -3 ) {


Note to self: we also need a standard utility function for within_box() or something similar.

We have box and rectangle with contains methods. We just need a helper function to make a rectangle from a centre point and bounds.

mlangsdorf · 2020-06-30T18:18:03Z

src/vehicle.cpp

-    const oter_id &cur_om_ter = overmap_buffer.ter( ms_to_omt_copy( g->m.getabs( global_pos3() ) ) );
+    // TODO: fix point types
+    const oter_id &cur_om_ter =
+        overmap_buffer.ter( tripoint_abs_omt( ms_to_omt_copy( g->m.getabs( global_pos3() ) ) ) );


not in the PR, but we also need a single function call that takes a local or absolute map square and returns the tripoint_abs_omt.

jbytheway · 2020-07-17T00:51:54Z

I've PRed all the separate parts I was able to pull out of this one. Unfortunately, there are still ~2000 lines of changes, so this is a bit of a beast to review. But it would be hard to split it up any more, so I'm marking it ready to review now.

As mentioned above, I tried to be as conservative as possible in my changes, so as to minimize the chance of introducing bugs, but with any change this large (especially after the messy rebase I just completed) there is of course some risk.

Nevertheless, I'm fairly confident I'm fixing more bugs than I'm introducing.

jbytheway · 2020-07-18T01:03:51Z

Working through some compile failures the slow way because they only fail on older compilers in Travis. Will ping here when they're resolved.

jbytheway · 2020-07-19T00:19:55Z

OK, looks good to me now. I think the remaining failures are not related to this PR.

Put the new point types through their paces by changing the types for overmap and overmapbuffer, and dealing with all the consequent fallout. Involves more changes than I originally anticipated, but serves as a good demonstration of the new point type features. Fixed some bugs along the way that were discovered by this new point type safety. The ones I remember are: * The evacuation shelter now correctly calculates the distance and direction to the refugee center, previously it was mostly nonsense (this only affects the message it prints out). * Weather conditions for NPC-planted crops are calculated more reasonably. Previously they were passing an overmap location as a map-square location, which could have caused all sorts of strangeness. * Translocators were similarly confusing map_square and overmap_terrain scales, so they might not have been working properly. * Two places were using map::points_in_radius which truncates to the map boundaries, but were using it for overmap-scale points, so it would only have worked correctly for some regions of the overmap.

kevingranade · 2020-07-20T07:43:18Z

In the interests of not making this drag on, we can do it later, but can't you forward declare in the header files?

jbytheway · 2020-07-20T10:54:54Z

Forward-declare what? The various point_foo_bar types? Not easily, since they're typedefs. If we want to take that approach we probably want a separate header to contain just a forward-declaration of the template and all the typedefs of it. (I'd probably call it coordinates_fwd.h). My assumption was that wouldn't really help because the vast majority of files will end up using points at some point and need the actual header included. But if you'd like me to create that then let me know.

When working on CleverRaven#41693 I totally forgot that I'd added shorter names for the various coord::scale values to use in project_to etc. without being so verbose. This makes the API more consistent because the names of the scales match the abbreviations in the type names. Convert to the shorter names. Strictly a find-and-replace commit, so hopefully should be safe.

When working on #41693 I totally forgot that I'd added shorter names for the various coord::scale values to use in project_to etc. without being so verbose. This makes the API more consistent because the names of the scales match the abbreviations in the type names. Convert to the shorter names. Strictly a find-and-replace commit, so hopefully should be safe.

kevingranade · 2020-07-23T01:00:48Z

I should have been more specific, mostly I was wincing at a new include in character.h

jbytheway · 2020-07-23T01:11:26Z

Well, that specific case might be avoidable in the short term, but sooner or later it'll be needed in creature.h (to store its position) too, so not much to be done about it.

Extra compile time is unfortunately a price we pay for these new types. If it starts to rear its head in the build stats then we can explore options, but for now I don't see it there so I'm going to hope it's a problem for another day.

ZhilkinSerg · 2020-11-07T13:13:55Z

src/overmapbuffer.cpp


-    for( x = 0, iter.x = origin.x - 1; x <= 2 ; ++iter.x, ++x ) {
-        for( y = 0, iter.y = origin.y - 1; y <= 2; ++iter.y, ++y ) {
+    for( int x = -1; x <= 1 ; ++x ) {


I think that is incorrect to use negative values for array indexes.

Well spotted; this is wrong.

I'll PR a fix.

See #45291.

Refactoring in CleverRaven#41693 accidentally introduced an out-of-bounds array access in overmapbuffer::scents_near. Fix it.

jbytheway requested a review from KorGgenT as a code owner June 30, 2020 01:53

ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Jun 30, 2020

mlangsdorf reviewed Jun 30, 2020

View reviewed changes

mlangsdorf approved these changes Jun 30, 2020

View reviewed changes

jbytheway mentioned this pull request Jul 1, 2020

Provide point types with coordinate system type-safety #32017

Merged

9 tasks

mlangsdorf mentioned this pull request Jul 2, 2020

map: refactor map class to get rid of optional z-levels #41758

Closed

jbytheway marked this pull request as draft July 8, 2020 13:31

jbytheway force-pushed the overmap_point_refactoring branch from 43de665 to 89def7c Compare July 17, 2020 00:43

jbytheway marked this pull request as ready for review July 17, 2020 00:48

jbytheway changed the title ~~[CR] Overmap point refactoring~~ Overmap point refactoring Jul 17, 2020

jbytheway force-pushed the overmap_point_refactoring branch 4 times, most recently from 573949f to dba15ff Compare July 17, 2020 20:44

jbytheway force-pushed the overmap_point_refactoring branch from dba15ff to 9a16301 Compare July 18, 2020 01:14

jbytheway force-pushed the overmap_point_refactoring branch from 9a16301 to c938bbd Compare July 20, 2020 00:57

kevingranade merged commit 3c67e80 into CleverRaven:master Jul 20, 2020

jbytheway deleted the overmap_point_refactoring branch July 20, 2020 10:55

jbytheway mentioned this pull request Jul 20, 2020

Use shorter scale specifications in project_to, etc. #42317

Merged

ZhilkinSerg reviewed Nov 7, 2020

View reviewed changes

jbytheway added a commit to jbytheway/Cataclysm-DDA that referenced this pull request Nov 7, 2020

Fix out of bounds array access

14973bb

Refactoring in CleverRaven#41693 accidentally introduced an out-of-bounds array access in overmapbuffer::scents_near. Fix it.

jbytheway mentioned this pull request Nov 7, 2020

Fix out of bounds array access #45291

Merged

jbytheway added a commit to jbytheway/Cataclysm-DDA that referenced this pull request Nov 8, 2020

Fix out of bounds array access

3783a24

Refactoring in CleverRaven#41693 accidentally introduced an out-of-bounds array access in overmapbuffer::scents_near. Fix it.

olanti-p mentioned this pull request Oct 14, 2021

[DDA Port] Overmap point refactoring + some bugfixes cataclysmbnteam/Cataclysm-BN#1053

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overmap point refactoring #41693

Overmap point refactoring #41693

jbytheway commented Jun 30, 2020 •

edited

Loading

mlangsdorf left a comment

mlangsdorf Jun 30, 2020

jbytheway Jun 30, 2020

mlangsdorf Jun 30, 2020

jbytheway Jun 30, 2020

mlangsdorf Jun 30, 2020

jbytheway commented Jun 30, 2020

mlangsdorf left a comment

mlangsdorf Jun 30, 2020

jbytheway Jun 30, 2020

mlangsdorf Jun 30, 2020

jbytheway commented Jul 17, 2020

jbytheway commented Jul 18, 2020

jbytheway commented Jul 19, 2020

kevingranade commented Jul 20, 2020

jbytheway commented Jul 20, 2020

kevingranade commented Jul 23, 2020

jbytheway commented Jul 23, 2020

ZhilkinSerg Nov 7, 2020

jbytheway Nov 7, 2020

jbytheway Nov 7, 2020

jbytheway Nov 7, 2020

Overmap point refactoring #41693

Overmap point refactoring #41693

Conversation

jbytheway commented Jun 30, 2020 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

mlangsdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway commented Jun 30, 2020

mlangsdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway commented Jul 17, 2020

jbytheway commented Jul 18, 2020

jbytheway commented Jul 19, 2020

kevingranade commented Jul 20, 2020

jbytheway commented Jul 20, 2020

kevingranade commented Jul 23, 2020

jbytheway commented Jul 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway commented Jun 30, 2020 •

edited

Loading