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

Sewer Features From DDA #626

Merged
merged 2 commits into from
Jul 24, 2021

Conversation

ZhamelSeh
Copy link
Contributor

@ZhamelSeh ZhamelSeh commented Jun 27, 2021

Summary

SUMMARY: Content "Bring over sewer extras from DDA."

Purpose of change

Bringing over changes for sewer extras as requested in: #98

Sewer change from DDA here: Cataclysm-DDA/pull/43326

Describe the solution

See purpose of change above

Describe alternatives you've considered

Redo all of it by hand.... suuure.

Testing

Made changes locally, loaded up a test game and wondered around sewers for a while.

Additional context

See 'Purpose of change' above, link to DDA pull request for more detailed information.
All credit goes to LovamKicsiGazsii for actual work performed.

Bringing over changes for sewer extras as requested in: cataclysmbnteam#98

Sewer change from DDA here:  CleverRaven/Cataclysm-DDA#43326
@ZhamelSeh ZhamelSeh marked this pull request as ready for review June 27, 2021 12:26
@@ -543,6 +543,32 @@
"mx_point_burned_ground": 125,
"mx_casings": 30
}
},
"sewer": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part unfortunately also requires porting some C++.
It might not be much, I'll check it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if you can point me in the right direction I'll get started on that this weekend when I get a chance, otherwise I'll take a look this weekend and see what I can find.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field itself is in regional_settings (.h and .cpp), but most of the handling of it is probably in overmap.cpp.

It would be the best if you found the actual DDA PR, since there may be other places. git blame is useful for this - git blame file_name_here shows which commit hash changed which line, you can then search for the commit on DDA github to find the PR.

If it's too complex to port, you can leave this PR open and I'll probably port the C++ part later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled the new sewer stuff directly from the PR (#43326) on DDA, which I named in the description, the only changes on that pull were to JSON files.

I did glance through the code and the only differences I could find were:
BN : overmap.cpp:1841-1842
tripoint omt_pos( i.pos, z ); tripoint sm_pos = omt_to_sm_copy( omt_pos );

DDA: overmap.cpp:1731-1732
tripoint_om_omt omt_pos( i.pos, z ); tripoint_om_sm sm_pos = project_to<coords::sm>( omt_pos );

These are referencing the CHUD mob and when to load it.

Unless there is something you know that I am missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled the new sewer stuff directly from the PR (#43326) on DDA, which I named in the description, the only changes on that pull were to JSON files.

In regional_settings.cpp, in void load_region_settings( const JsonObject &jo ), there are 3 lines:

        load_building_types( "houses", new_region.city_spec.houses );
        load_building_types( "shops", new_region.city_spec.shops );
        load_building_types( "parks", new_region.city_spec.parks );

DDA probably also has something like

        load_building_types( "sewers", new_region.city_spec.sewers );

in there.
That line and PR that introduced it are needed for the "sewers" array in the JSON to be handled. Without it, it will be ignored and reported as a JSON field that wasn't read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be glad to be wrong here.

Bottom line is: does it clearly show up as expected in manual testing? If it does, it works well enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A screenshot from build a40a4de, 2021-06-23-1851:
CBN-1

And a screenshot from the same build, but from my test game directory, after the changes:
CBN-2

I always keep one directory for actually playing, and a separate directory for testing changes. Easier to compare the two, at least for me, this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anything else needed from me on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I totally forgot that this should work. Sorry.
I'll test+possibly merge in next batch, most likely tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Ready to move on to other things :)

@Coolthulhu Coolthulhu self-assigned this Jul 23, 2021
@Coolthulhu Coolthulhu merged commit 6ed9af3 into cataclysmbnteam:upload Jul 24, 2021
@ZhamelSeh ZhamelSeh deleted the Sewer-Features-from-DDA branch July 24, 2021 23:25
@olanti-p olanti-p mentioned this pull request Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants