Skip to content

change planer algorithms to match the original #1908

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.

Sign up for GitHub

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

Open
libv wants to merge 1 commit into
base: master
Choose a base branch
from
Open

change planer algorithms to match the original #1908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion libs/s25main/buildings/noBuildingSite.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ noBuildingSite::noBuildingSite(const BuildingType type, const MapPoint pos, cons
|| GetSize() == BuildingQuality::Harbor)
{
// Höhe auf dem Punkt, wo die Baustelle steht
int altitude = world->GetNode(pos).altitude;
int altitude = world->GetNode(this->GetFlagPos()).altitude;
Copy link
Copy Markdown
Member

@Flamefire Flamefire Mar 28, 2026

Choose a reason for hiding this comment

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

To make this easier to understand:

Suggested change
int altitude = world->GetNode(this->GetFlagPos()).altitude;
const auto flagAltitude = world->GetNode(this->GetFlagPos()).altitude;


if(altitude - world->GetNode(pos).altitude != 0)
state = BuildingSiteState::Planing;
Comment on lines +34 to +35
Copy link
Copy Markdown
Member

@Flamefire Flamefire Mar 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Enhance check (same below) and indent

Suggested change
if(altitude - world->GetNode(pos).altitude != 0)
state = BuildingSiteState::Planing;
if(altitude != world->GetNode(pos).altitude)
state = BuildingSiteState::Planing;

And put the loop into an else-branch

Copy link
Copy Markdown
Author

@libv libv Mar 28, 2026

Choose a reason for hiding this comment

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

I actually removed the tabbed indents by hand, as my editor is set to kernel style. And the very awkward calculation matches the rest of the code. So when reviewing this patch, please see the context, i have done extra work to match the rest of the codebase.

Copy link
Copy Markdown
Member

@Flamefire Flamefire Mar 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I now see there is a tab left, yes. You can use clang-format to auto-format. If it is found by configure there is even a build target for that. CI will tell you too once run

This wasn't criticism of your work and matching this (strange) style is certainly right in general.

My suggestion here is to fix/use the != style for the new code and adapt the old code below to match this. I see no reason for keeping the old style which raises questions e.g. if it could run into UB. Doesn't here, but != avoids the question and makes the intent clearer.

Copy link
Copy Markdown
Author

@libv libv Mar 28, 2026

Choose a reason for hiding this comment

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

Well, from where I sit, once you start refactoring code, you never stop :) I would just use the old style here and let it be until someone uses lindent on whole files.


for(const auto dir : helpers::EnumRange<Direction>{})
{
Expand Down Expand Up @@ -377,6 +380,8 @@ void noBuildingSite::PlaningFinished()
state = BuildingSiteState::Building;
planer = nullptr;

world->ChangeAltitude(pos, world->GetNode(this->GetFlagPos()).altitude);

// Wir hätten gerne einen Bauarbeiter...
world->GetPlayer(player).AddJobWanted(Job::Builder, this);

Expand Down
2 changes: 1 addition & 1 deletion libs/s25main/figures/nofPlaner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ void nofPlaner::HandleDerivedEvent(const unsigned id)
{
// Planieren (falls Baustelle noch existiert)
if(building_site)
world->ChangeAltitude(pos, world->GetNode(building_site->GetPos()).altitude);
world->ChangeAltitude(pos, world->GetNode(building_site->GetFlagPos()).altitude);
/// Sounds abmelden
world->GetSoundMgr().stopSounds(*this);

Expand Down
Loading