-
-
Notifications
You must be signed in to change notification settings - Fork 946
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
Some NewGRF variables concerning railtypes #7000
Conversation
NML usage: "var[0x6a, 8, 0xFF, ELRL]" |
Tested, worked as expected for me. |
The intent is to use this in, varaction 2 for e.g. CB36. Without this var, railtype labels have to be checked individually, which:
|
Open questions (that i can't answer myself):
|
I second the "skip the redundant part". This variable seems to be about comparing two railtypes.
In addition it may make sense to extend 4A with more attributes about the present railtype:
Maybe - but not necessarily - related:
|
i don't think "track has catenary" is a property of a railtype that can be meaningfully extracted from the arbitrary NewGRF data.
my previous idea about that was a vehicle state variable with values:
possibly also add a bitset of what the reason for "cruising" is?
i feel like there was another ticket where that discussion would fit better independent of that, adding railtype max speed to var4A should be possible, however |
Catenary is defined by RTF_CATENARY. |
Ok, but if we open that can of worms, people will request to know about 3rd rail. and the ability to issue sparks from somewhere other than the top of the vehicle, and... (and by "people" i mean myself) |
So...is it easier if I just continue checking 'ELRL' and ignore NuTracks and friends? o_O |
f01b3db
to
2560ec4
Compare
5b674bd
to
0ae48df
Compare
Removed the lookup bit, and renamed the var to 63, because there's not enough relation to var 4A left to warrant a similarity in name. Added "has catenary" flag and railtype speed limit to var 4A (doesn't have to be the speed limit currently affecting the train, so i don't know how useful that is) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, will wait for a GRF-God to approve & merge
We'll also want some docs once this is merged. Such admin :) |
So, i have no idea if this is the correct way to make a translation between grf railtype and game railtype. Also, i have added a variable that makes that translation and then a reverse translation, this might be useful for checking whether two "logical" railtypes are mapped to the same internal railtype. this might need some more extensive testing |
Hmm, why does a NewGRF need to check mappings? It should be transparent to it. |
Well, my use case would be a wagon that can run on a 15t, 18t, 20t or 22t railtype, with different capacities. but if the railtype GRF doesn't provide any distinguished weight class types (as per the standardized railtype scheme), the different variations could be omitted. |
Last comment from a NewGRF author is over a year ago. |
uhm wtf kind of reasoning is that?!? |
As a NewGRF Author, I demand this |
Needs a rebase. And I do think that it can improve handling of dual/multi-mode engines for NewGRFs. |
This could have been a simple single-issue change a long time ago, and I'd have used it. But I know that stacking single-issue changes is how we get an incoherent newgrf spec. Stability vs. innovation eh. |
rebased, and removed the Var 3F part that was probably too hacky |
5279724
to
c0e9cb7
Compare
also added range checks for Action 7 |
Principal approval from me. |
@@ -704,6 +704,12 @@ static uint32 VehicleGetVariable(Vehicle *v, const VehicleScopeResolver *object, | |||
return ret; | |||
} | |||
|
|||
case 0x63: { // Poweredness relative to arbitrary railtype | |||
if (v->type != VEH_TRAIN) return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the use-case for this is: A vehicle is dual-powered, it is powered on normal rail, but on electrified rail it raises a pantograph.
The same use-case is valid for roadtypes.
} | ||
RailType rt1 = GetRailTypeByLabel(_cur.grffile->railtype_list[r1]); | ||
RailType rt2 = GetRailTypeByLabel(_cur.grffile->railtype_list[r2]); | ||
result = rt1 != INVALID_RAILTYPE && rt2 != INVALID_RAILTYPE && rt1 != rt2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the negation of condition 0x13, which makes it weird in corner cases, and hard to implement in NML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is intentional. in case one of the railtypes is undefined, both conditions return "false"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i'm too deep in formal logic here, but comparing <undefined>
to <undefined>
should never return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and since we can't return <undefined>
, the only option left is to return false
uint r1 = GB(cond_val, 0, 8); | ||
uint r2 = GB(cond_val, 8, 8); | ||
if (r1 >= _cur.grffile->railtype_list.size() || r2 >= _cur.grffile->railtype_list.size()) { | ||
result = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar places issue a grfmsg() in this case. Though not sure whether this case always triggers in GRF loading stages before reservation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll take suggestions on what debuglevel this should be shown.
as for earlier loading stages, i guess that applies to action 9 only? not sure if the check makes any sense there.
For some reason I can't reply to the first comment from frosch: Roadtypes don't distinguish compatible and powered. If an RV can drive on it, it is by definition also powered and vice versa. |
i kinda see his point though. historically, there have been busses which can run on diesel or electric (catenary) mode, and maybe should make that visible. |
@Eddi-z After further though, yes. So that var should preferably also work for RVs. |
if (v->type != VEH_TRAIN) return 0; | ||
if (parameter >= RAILTYPE_END) return 0; | ||
RailType rt = GetTileRailType(v->tile); | ||
return HasPowerOnRail(object->ro.grffile->railtype_map[parameter], rt) ? 1 : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm suddenly unsure whether i should have used railtype_list instead here, like in the action7 changes
I created a new PR #8554 to implement the ideas from this PR. Please put more effort into thinking what a PR should do, and what cases must be considered, before coding and submitting. This helps missing 90% of the things to consider. |
Add: Var 6A, a clone of Var 4A for querying poweredness compared to an arbitrary railtype