Add: NewGRF road stops #10144

JGRennison · 2022-11-06T15:42:39Z

Motivation / Problem

Add NewGRF road stops.
Loosely based on #7955. but with various things added, removed, changed, fixed, etc. The NewGRF interfaces do not match.

Description

The implementation and NewGRF interface broadly matches the implementation in the JGR patchpack branch.

However various things have been removed as they are not applicable to this branch:

Road waypoints (however the WAYP class ID is still reserved)
One way road stops support
Bridge over road stops support
Feature detection, name/ID mapping, etc.

The interface is as described here, except without all the JGR patchpack specific stuff, as described above.
The feature is allocated to the ID: 0x14.
Specifically the following have been removed with respect to the above interface description:

Bit 2 of property C
Bit 1 of property 12
Properties 13 and 14
Value 2 of variable 41
Variable 50
Bits 21 - 22 of variable 68
Variable 6B (can be added if interest exists)

The NML interface is described here, with the same set of items removed as listed above.
This corresponds to: RST_DRAW_FLAG_WAYPOINT_GROUND, RST_GENERAL_FLAG_NO_ONE_WAY_OVERLAY, minimum_bridge_height, disallowed_bridge_pillars, RST_TYPE_WAYPOINT, one_way_info, nearby_tile_one_way_info, nearby_tile_road/tram variables.

A branch which implements this can be found here: here (upstream-road-stops branch).

Limitations

This cannot be used with existing road stop GRFs which target the JGR patchpack branch.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

The bug fix is important enough to be backported? (label: 'backport requested')
This PR touches english.txt or translations? Check the guidelines
This PR affects the save game format? (label 'savegame upgrade')
This PR affects the GS/AI API? (label 'needs review: Script API')
- ai_changelog.hpp, gs_changelog.hpp need updating.
- The compatibility wrappers (compat_*.nut) need updating.
This PR affects the NewGRF API? (label 'needs review: NewGRF')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

JGRennison · 2022-11-06T15:43:33Z

roadstops_test_nml.zip

Test GRF with very ugly graphics

PeterN

Quick initial skim...

src/saveload/station_sl.cpp

src/station.cpp

src/cheat_gui.cpp

stormcone · 2022-12-17T20:17:16Z

src/newgrf_animation_base.h

 /**
 * Helper class for a unified approach to NewGRF animation.
 * @tparam Tbase       Instantiation of this class.
 * @tparam Tspec       NewGRF specification related to the animated tile.
 * @tparam Tobj        Object related to the animated tile.
 * @tparam Textra      Custom extra callback data.
 * @tparam GetCallback The callback function pointer.
+ * @tparam Tframehelper The animation frame get/set helper.


It breaks the indentation. :)

stormcone · 2022-12-17T20:21:39Z

src/newgrf.cpp

@@ -4707,6 +4708,131 @@ static ChangeInfoResult AirportTilesChangeInfo(uint airtid, int numinfo, int pro
 	return ret;
 }

+/**
+ * Ignore properties for roadstops


Suggested change

* Ignore properties for roadstops

* Ignore properties for roadstops.

Although the period seems to be used mixed at the end of sentences.

See the other comments in this file and the wider codebase

stormcone · 2022-12-17T20:32:37Z

src/road_gui.cpp

+	GUIRoadStopClassList roadstop_classes;     ///< Available road stop classes.
+	StringFilter string_filter;              ///< Filter for available road stop classes.
+	QueryString filter_editbox;              ///< Filter editbox.
+


There are mixed signals above whether the comments should be aligned or not. :)

src/newgrf_roadstop.h

reldred · 2022-12-27T22:07:10Z

I'm not liable to compile this any time soon for this implementation, but I've coded up Fridaemons warehouses as roadstops in NML over here: https://github.com/reldred/re_foars

Code is nothing fancy, but removing the JGRPP specific code it should, I'm told, compile fine.

I know I haven't specified a license in the repo, but it inherits GPLv2 by virtue of Fridaemons sprites.

JGRennison · 2022-12-27T22:31:34Z

re_foars.zip
Above GRF built for this PR

glx22 · 2022-12-27T22:41:04Z

I triggered a release build for this PR, will be available on https://www.openttd.org/downloads/openttd-branches/pr10144/latest when done.

src/economy.cpp

PeterN · 2022-12-27T22:49:22Z

src/newgrf_roadstop.h

+enum RoadStopClassID : byte {
+	ROADSTOP_CLASS_BEGIN = 0,    ///< The lowest valid value
+	ROADSTOP_CLASS_DFLT = 0,     ///< Default road stop class.
+	ROADSTOP_CLASS_WAYP,         ///< Waypoint class.


Are waypoints relevant for road? Reserved for future (and jgrpp) use I suppose.

Road waypoints is already implemented with custom road stops support in my branch.
Not reserving the class ID would make adding this to vanilla in future a bit less straightforward.
I am not precious about removing that if it isn't wanted though.

Reserving it is fine imho, maybe with a note in the comment.

PeterN · 2022-12-27T22:50:02Z

src/newgrf_roadstop.h

+
+	RoadStopScopeResolver(ResolverObject& ro, BaseStation* st, const RoadStopSpec *roadstopspec, TileIndex tile, RoadType roadtype, StationType type, uint8 view = 0)
+		: ScopeResolver(ro), tile(tile), st(st), roadstopspec(roadstopspec), type(type), view(view), roadtype(roadtype)
+		{


Indentation

PeterN · 2022-12-27T22:50:17Z

src/newgrf_roadstop.h

+	RoadStopResolverObject(const RoadStopSpec* roadstopspec, BaseStation* st, TileIndex tile, RoadType roadtype, StationType type, uint8 view, CallbackID callback = CBID_NO_CALLBACK, uint32 param1 = 0, uint32 param2 = 0);
+	~RoadStopResolverObject();
+
+	ScopeResolver* GetScope(VarSpriteGroupScope scope = VSG_SCOPE_SELF, byte relative = 0) override {


Newline before {

PeterN · 2022-12-27T22:56:47Z

src/base_station_base.h


 	TileArea train_station;         ///< Tile area the train 'station' part covers
 	StationRect rect;               ///< NOSAVE: Station spread out rectangle maintained by StationRect::xxx() functions

+	std::vector<TileIndex> custom_road_stop_tiles;      ///< List of custom road stop tiles
+	std::vector<uint16> custom_road_stop_data;          ///< Custom road stop random bits (low) and animation byte (high) in same order as custom_road_stop_tiles


Would this be better as a single vector containing a struct?

This makes saveload with the new system a bit less straightforward, I should be able to do that later though.

src/station.cpp

src/station_cmd.cpp

JGRennison · 2022-12-31T22:40:30Z

The savegame format has changed since the last update of this PR as per comments above.

PeterN

Not completely reviewed, just some naming ideas.

PeterN · 2023-01-22T18:38:56Z

src/newgrf_roadstop.cpp

+	return specindex < st->roadstop_speclist.size() ? st->roadstop_speclist[specindex].spec : nullptr;
+}
+
+int AllocateRoadStopSpecToStation(const RoadStopSpec *statspec, BaseStation *st, bool exec)


AllocateSpecToRoadStop might be less wordy?

PeterN · 2023-01-22T18:39:13Z

src/newgrf_roadstop.cpp

+	return i;
+}
+
+void DeallocateRoadStopSpecFromStation(BaseStation *st, byte specindex)


Similarly, DeallocateSpecFromRoadStop?

PeterN · 2023-01-22T20:02:04Z

src/newgrf_roadstop.cpp

+ * Update the cached animation trigger bitmask for a station.
+ * @param st Station to update.
+ */
+void StationUpdateRoadStopCachedTriggers(BaseStation *st)


RoadStopUpdateCachedTriggers?

src/station_cmd.cpp

src/road_gui.cpp

LordAro

A few things, but nothing merge-blocking

LordAro · 2023-02-26T20:05:35Z

src/newgrf_roadstop.cpp

+		/* Road type */
+		case 0x43: return get_road_type_variable(RTT_ROAD);
+
+		/* Tram type */
+		case 0x44: return get_road_type_variable(RTT_TRAM);


I'd be tempted to use fallthrough & a RoadTramType rtt = 0x43 ? RTT_ROAD : RTT_TRAM variable personally, but eh

LordAro · 2023-02-26T20:08:30Z

src/road_gui.cpp

+	/** Sort classes by RoadStopClassID. */
+	static bool RoadStopClassIDSorter(RoadStopClassID const &a, RoadStopClassID const &b)
+	{
+		return a < b;
+	}


is this function even needed? Seems pretty trivial...

LordAro · 2023-02-26T20:10:55Z

src/table/newgrf_debug_data.h

+	bool IsInspectable(uint index) const override        { return GetRoadStopSpec(index) != nullptr; }
+	uint GetParent(uint index) const override            { return GetInspectWindowNumber(GSF_FAKE_TOWNS, BaseStation::GetByTile(index)->town->index); }
+	const void *GetInstance(uint index)const override    { return nullptr; }
+	const void *GetSpec(uint index) const override       { return GetRoadStopSpec(index); }
+	void SetStringParameters(uint index) const override  { this->SetObjectAtStringParameters(STR_STATION_NAME, GetStationIndex(index), index); }
+	uint32 GetGRFID(uint index) const override           { return (this->IsInspectable(index)) ? GetRoadStopSpec(index)->grf_prop.grffile->grfid : 0; }


some unusual alignment here :)

andythenorth · 2023-09-14T07:03:05Z

Updated NewGRF spec status https://wiki.openttd.org/en/Development/NewGRF/Specification%20Status

andythenorth · 2023-09-16T08:31:48Z

JGRennison · 2023-09-16T09:39:40Z

Additions to the above:
Action 0 property 12, bits 1, 7, 8 are not implemented
Action 0 property 16 is not implemented
Action 2 variable 68, bits 21-22 and 24-31 are not implemented

2TallTyler added the size: large This Pull Request is large in size; review will take a while label Nov 6, 2022

JGRennison force-pushed the upstream_road_stops branch from 1dd9d9b to c4dd2d5 Compare November 6, 2022 18:06

PeterN reviewed Nov 6, 2022

View reviewed changes

src/saveload/station_sl.cpp Outdated Show resolved Hide resolved

src/saveload/station_sl.cpp Outdated Show resolved Hide resolved

src/station.cpp Outdated Show resolved Hide resolved

JGRennison force-pushed the upstream_road_stops branch from c4dd2d5 to 7d2a390 Compare November 6, 2022 18:28

michicc added component: NewGRF This issue is related to NewGRFs needs review: NewGRF Review requested from a NewGRF expert labels Nov 6, 2022

JGRennison marked this pull request as ready for review December 15, 2022 18:07

JGRennison changed the title ~~Add: NewGRF road stops~~ Draft: Add: NewGRF road stops Dec 15, 2022

JGRennison marked this pull request as draft December 15, 2022 18:13

JGRennison force-pushed the upstream_road_stops branch 2 times, most recently from e2af225 to 7d9f480 Compare December 17, 2022 01:13

JGRennison changed the title ~~Draft: Add: NewGRF road stops~~ Add: NewGRF road stops Dec 17, 2022

JGRennison marked this pull request as ready for review December 17, 2022 01:20

stormcone reviewed Dec 17, 2022

View reviewed changes

JGRennison force-pushed the upstream_road_stops branch from 7d9f480 to e675907 Compare December 17, 2022 21:23

JGRennison force-pushed the upstream_road_stops branch from e675907 to 8fe2f52 Compare December 27, 2022 21:57

PeterN reviewed Dec 27, 2022

View reviewed changes

JGRennison force-pushed the upstream_road_stops branch from 8fe2f52 to f4de070 Compare December 27, 2022 23:23

2TallTyler mentioned this pull request Dec 29, 2022

WIP Feature: NewGRF Road Stops (Feature 14) #7955

Closed

6 tasks

JGRennison force-pushed the upstream_road_stops branch from f4de070 to e7b592e Compare December 31, 2022 22:39

PeterN reviewed Jan 22, 2023

View reviewed changes

JGRennison force-pushed the upstream_road_stops branch from e7b592e to 8cd073f Compare January 29, 2023 23:00

github-advanced-security bot found potential problems Jan 29, 2023

View reviewed changes

src/station_cmd.cpp Fixed Show fixed Hide fixed

src/road_gui.cpp Fixed Show fixed Hide fixed

Add: NewGRF road stops

27676a3

JGRennison force-pushed the upstream_road_stops branch from 8cd073f to 27676a3 Compare January 30, 2023 00:18

LordAro approved these changes Feb 26, 2023

View reviewed changes

rubidium42 merged commit 4c1406a into OpenTTD:master Feb 26, 2023

JGRennison mentioned this pull request Feb 28, 2023

Add: Road stops (feature 0x14) OpenTTD/nml#279

Merged

JGRennison mentioned this pull request Jul 10, 2023

Fix: Build road/tram stop windows did not set WindowDesc::ini_key #11126

Merged

JGRennison deleted the upstream_road_stops branch January 9, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: NewGRF road stops #10144

Add: NewGRF road stops #10144

JGRennison commented Nov 6, 2022 •

edited

JGRennison commented Nov 6, 2022

PeterN left a comment

stormcone Dec 17, 2022

stormcone Dec 17, 2022

JGRennison Dec 17, 2022

stormcone Dec 17, 2022

reldred commented Dec 27, 2022

JGRennison commented Dec 27, 2022

glx22 commented Dec 27, 2022

PeterN Dec 27, 2022

JGRennison Dec 27, 2022 •

edited

PeterN Dec 27, 2022

PeterN Dec 27, 2022

PeterN Dec 27, 2022

PeterN Dec 27, 2022

JGRennison Dec 27, 2022

JGRennison commented Dec 31, 2022

PeterN left a comment

PeterN Jan 22, 2023

PeterN Jan 22, 2023

PeterN Jan 22, 2023

LordAro left a comment

LordAro Feb 26, 2023

LordAro Feb 26, 2023

LordAro Feb 26, 2023

andythenorth commented Sep 14, 2023

andythenorth commented Sep 16, 2023 •

edited

JGRennison commented Sep 16, 2023

	* Ignore properties for roadstops
	* Ignore properties for roadstops.

Add: NewGRF road stops #10144

Add: NewGRF road stops #10144

Conversation

JGRennison commented Nov 6, 2022 • edited

Motivation / Problem

Description

Limitations

Checklist for review

JGRennison commented Nov 6, 2022

PeterN 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

reldred commented Dec 27, 2022

JGRennison commented Dec 27, 2022

glx22 commented Dec 27, 2022

Choose a reason for hiding this comment

JGRennison Dec 27, 2022 • edited

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

JGRennison commented Dec 31, 2022

PeterN 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

LordAro 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

andythenorth commented Sep 14, 2023

andythenorth commented Sep 16, 2023 • edited

JGRennison commented Sep 16, 2023

JGRennison commented Nov 6, 2022 •

edited

JGRennison Dec 27, 2022 •

edited

andythenorth commented Sep 16, 2023 •

edited