[patch] Regression tests for elevated ways before refactoring tool_build_way_t

[victor_18993]

Before touching tool_build_way_t, I wanted a proper regression net for elevated ways.

Right now elevated ways have no dedicated tests, so moving that code would mean refactoring without knowing whether the behaviour changed. This patch adds 26 tests and does not touch production code.

The tests cover straight routes, bends, water, roads, stations, slopes, map edges, ownership, scenario rules, maintenance, rebuilding and removal. I measured every expected result on an unmodified build before turning it into an assertion.

A few current behaviours look odd — for example, funds are not checked, a single slope is rejected while a double slope is accepted, and an elevated way can climb over a bridge deck — but this patch deliberately keeps them unchanged. If any of those are bugs, I would rather handle them separately.

Current state:

Code Select Expand

26/26 new tests
232/232 full suite
git diff --check clean
Only tests/all_tests.nut and the new test_way_elevated.nut are changed.

Question for prissi

Prissi, before I move any production code, does this test baseline cover the elevated-way behaviour you want preserved?

For the later separation, my preference is to keep TOOL_BUILD_WAY as the pakset and network entry point and separate the different rules internally. Giving elevated ways a new tool ID would require every pakset to update its menuconf.tab.

Is that the direction you had in mind, or would you prefer elevated ways to become a separate tool despite the pakset compatibility cost?

[prissi]

The maglev tests of pak64 are elevated ways, but I did not look into the tests in detail. I must admit that I never programmed tests until they were submitted.

On the different tools, fill_menu could automatically assign elevated ways a different tool than normal ways. That seems rather easy?

[victor_18993]

Thanks, Prissi.
Yes, if

Code Select Expand

fill_menu() can assign elevated ways a separate tool automatically, that would avoid the pakset compatibility problem I was concerned about. I was assuming that introducing a new tool ID would require existing

Code Select Expand

menuconf.tab files to be updated manually.
I will review the current

Code Select Expand

fill_menu() path and see whether elevated way descriptors can be routed to a separate tool there while keeping the existing pakset definitions unchanged.
If that works cleanly, I agree that separating the elevated-way tool would probably give us a much better boundary for the later refactoring than keeping everything inside

Code Select Expand

tool_build_way_t.
I will keep the current regression patch independent from that work so that the baseline remains unchanged while the production-code split is developed separately.

[victor_18993]

prissi, your remark about separating the way tool sent me looking, and here is a first cut. It is
structural only: no elevated behaviour has moved yet, and nothing a player or a pakset can see
changes. Patch attached against r12130, for review rather than for committing.

fill_menu() turned out not to be the place

It only does add_tool_selector(desc->get_builder()). The tool itself is built one per descriptor in
way_builder_t::register_desc(), and that is where a descriptor can be given a different one.

But there is a second path, and it matters more: create_general_tool() picks the class from the tool
id alone, and the network (nwc_tool_t::init_tool), the script API and a menuconf.tab line of the form
general_tool[14],<param> all go through it. Since the automated tests build ways through the script
API, a subclass known only to the menu would have been invisible to every test - and worse, anything
moved into it later would not have run on the network at all. So the factory now takes the parameter
too, and all three paths rebuild the same class the menu holds.

No new tool id

I looked at giving elevated ways an id of their own and dropped it. Scenario rules index by tool id -
is_tool_allowed() and is_work_allowed_here() - and the script API exposes the same identity as
tool_build_way. An elevated way with its own id would fall outside the rules that cover it today,
which is an observable change of behaviour, and menuconf.tab numbering would follow. So the cut is
tool_build_elevated_way_t : public tool_build_way_t with the same TOOL_BUILD_WAY.

The base class still decides

This is the part I would most like your opinion on. The factory cannot always resolve a descriptor:
a numeric way type parameter resolves through a mutable defaults table, so it is not knowable at
construction time. The specialisation is therefore assigned when the descriptor is known, and the
base class keeps the descriptor-derived predicate, so a way that ends up with the general tool is
built exactly as before.

That is deliberate - it makes the cut unable to change behaviour - but it also means the subclass is
a boundary today and not yet a condition for elevated ways to work. Moving the rules into it is the
next step, not this one. The six places that asked whether the way is elevated now go through one
is_elevated(), which is where they would move from.

Testing

Full suite 263/263 before and after, and the [n/m] sequence is identical line by line. That count
includes the 26 elevated characterisation tests I posted in this thread earlier, plus the two
elevated tests the trunk gained in r12129. No new warnings: 96 files recompile either way and both
builds produce the same 16.

Since the separation cannot be seen from a script - if it could, it would be a behaviour change - I
measured it instead with a temporary probe in the factory and counted where each descriptor went:
elevated_monorail and MaglevTrackElevated to the elevated tool; runway_grass, which has the same
system type but is an air way, to the general one, along with flat monorail, Powerline and the roads.
I also printed the tool id of every registered way tool: all 4110, so the id really is unchanged. The
probes were removed before the final build and the final run.

Compatibility

• Network: nwc_tool_t::rdwr is untouched, so the bytes on the wire are the same. Only the local

reconstruction changed, and both sides do it the same way.

• Savegames: no tool ids are stored, nothing to change.
• Paksets and menuconf.tab: unchanged. The ways(waytype,systemtype) path names no ids.
• Scenario rules and the script API keep TOOL_BUILD_WAY.

One known limit, which I left alone on purpose: a menuconf.tab that uses general_tool[14] with a
numeric way type still gets the general tool. That is what happens today and it works, because the
base class carries the elevated logic.

Nothing is integrated and I have not touched the test patch collision - the trunk now has its own
tests/tests/test_way_elevated.nut from r12129, so my earlier test patch needs renaming before it can
apply, and that is a separate matter from this one.

What I would like to know is whether this is the boundary you had in mind, and whether you would
rather the base class stop deciding at all - which would mean either handling the numeric parameter
in the factory, or accepting that ways reached that way lose the specialisation. I would rather agree
on that before moving any rules into the subclass.

[prissi]

Thank you for investigating this.

However, if there is no new tool id, which you have made clear does not work as I had imagined, then would it be not easier to distinguish insed the waybuilder tool and then call there waybuild/elevated_waybuilder/powerline_builder inside the waytool? Or even a work_powerline/work_way/work_elevated function to separate the logic paths?

[victor_18993]

Before writing another patch I went to look at where that split would go, and I think it may already
be there, one layer below the tool.

way_builder_t::build() already dispatches by type - build_elevated(), build_road(), build_track(),
build_powerline(), build_river() - and calc_route() picks between intern_calc_route_elevated(),
intern_calc_route_runways() and the normal one. I put a temporary probe on that dispatcher and ran
the test suite to check it is really used and not just present: roads go to build_road, tracks and
flat monorail and runways to build_track, Powerline to build_powerline, and elevated monorail to
build_elevated plus build_track.

So a waybuild/elevated_waybuilder/powerline_builder layer inside tool_build_way_t would sit on top of
one that already does that job, and I did not want to add it without asking you first.

What is left in the tool itself is small: a handful of conditions in start_at, is_valid_pos,
calc_route and mark_tiles. The awkward part is that they sit inside functions that are otherwise
shared. mark_tiles is the clearest example - most of it is the diagonal image selection, which is the
same for every way type, while the elevated part is two lines that add a height offset. Splitting
that into three would either duplicate the shared body or move large blocks around without creating a
real boundary, so I stopped rather than send you that.

The subclass I tried before does not look necessary under this direction either. It was useful for
finding out how tools get rebuilt for network and scripts, and why a new tool id causes trouble, but
I am not going to defend the design itself if it is not what we want.

The one place where I did find a strong separation by type is way_builder_t::is_allowed_step(). It is
a big function, roughly 440 lines, with distinct branches for road, track, tram, powerline, water,
river and air, and those really are different construction rules rather than the same rule with a
flag. That looks like a more natural candidate for is_allowed_step_road/track/powerline and so on.
It is also a lot more delicate: the AIs, the city road builder and the river generator all go through
it, and we do not have a strong enough determinism check yet to touch it blind.

Unless I misunderstood what you meant, in which case just say so and I will look again.

Which direction would you prefer: leave the tool as it is and do the separation in way_builder_t,
separate some specific part of tool_build_way_t after all, or did you have a different area in mind?

[prissi]

I think I own an apology. Indeed, the long switch in check_allowed_step (and a few other places) were the ones that I had in mind if one could not cleanly separate the whole thing. Especially the powerline tram, and elevated was a special condition almost everywhere.

Maybe one needs to do more planning before starting to code. What would be a clean good OOP structure> Maybe a way builder class which has subclasses overriding certain functions (calc_route and check_allowed step for instance)?

At the moment, changing a little here may have unintended side effect on otyher things easily and some functions are old style long switches. The different buildings routines are also derived not from classes but again from waytypes. This function clearly tells of its legacy and was extended many times with hacks for trams, elevated ways, rivers, and powerlines.

One the other hand, it works for now. So maybe putting this back until after a release and rather plan a better organisation of the code to have proper OOP and not this mixture.

[victor_18993]

Thanks, Prissi.
That makes sense to me.
The investigation confirmed that the real problem is broader than one isolated branch: elevated ways, powerlines, trams and the other special cases are spread across several parts of the builder, so splitting one function now could easily create a cleaner-looking local change while making the overall structure harder to maintain.
I agree that it is better not to force this refactoring before the next release.
In the meantime, I can use the analysis we already have to document a possible OOP structure for the builder — for example, keeping a common base and separating the parts that genuinely differ, such as route calculation and

Code Select Expand

is_allowed_step() — without touching production code yet.
That should give us something concrete to review later, once the release is out and there is more room for a deeper refactoring.