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

Some NewGRF variables concerning railtypes #7000

Closed
wants to merge 3 commits into from
Closed

Conversation

@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Dec 29, 2018

Add: Var 6A, a clone of Var 4A for querying poweredness compared to an arbitrary railtype

@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Dec 29, 2018

NML usage: "var[0x6a, 8, 0xFF, ELRL]"

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Dec 29, 2018

Tested, worked as expected for me.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 3, 2019

The intent is to use this in, varaction 2 for e.g. CB36.

Without this var, railtype labels have to be checked individually, which:

  1. is potentially a long list
  2. requires vehicle grf to know all possible railtypes in advance, rather than relying on railtype grf to specify compatibility and powered-ness
@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Jan 3, 2019

Open questions (that i can't answer myself):

  • choice of variable number?
  • skip the (redundant, because same as var4A) reverse lookup?
  • are there railtype introspection variables needed other than poweredness?
@frosch123
Copy link
Member

@frosch123 frosch123 commented Jan 7, 2019

I second the "skip the redundant part".

This variable seems to be about comparing two railtypes.
Besided poweredness there is not much to compare (compatibility is useless here).

  • Which one is better? Higher speed limit or something? Newer intro date?

In addition it may make sense to extend 4A with more attributes about the present railtype:

  • Track has catenary?

Maybe - but not necessarily - related:

  • Sometimes vehicles want to know about their speed limit, and whether it is defined by the vehicle, the railtype, the bridge, the timetable, stopping, ...
    Currently there are some acceleration/deceleration flags, but an actual number to compare with "current_speed" may be more useful.
@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Jan 7, 2019

In addition it may make sense to extend 4A with more attributes about the present railtype:

* Track has catenary?

i don't think "track has catenary" is a property of a railtype that can be meaningfully extracted from the arbitrary NewGRF data.

Maybe - but not necessarily - related:

* Sometimes vehicles want to know about their speed limit, and whether it is defined by the vehicle, the railtype, the bridge, the timetable, stopping, ...
  Currently there are some acceleration/deceleration flags, but an actual number to compare with "current_speed" may be more useful.

my previous idea about that was a vehicle state variable with values:

  • standing
  • accelerating
  • cruising (at max speed)
  • decelerating
  • unloading loading

possibly also add a bitset of what the reason for "cruising" is?

  • resulting acceleration force was (near) 0 (vehicle power exhausted)
  • railtype limit
  • bridge limit
  • curve limit
  • ...?

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

@frosch123
Copy link
Member

@frosch123 frosch123 commented Jan 7, 2019

Catenary is defined by RTF_CATENARY.

@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Jan 7, 2019

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)

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 7, 2019

So...is it easier if I just continue checking 'ELRL' and ignore NuTracks and friends? o_O

@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch 5 times, most recently from f01b3db to 2560ec4 Jan 15, 2019
@Eddi-z Eddi-z changed the title Add: Var 6A, a clone of Var 4A for querying poweredness compared to a… Some NewGRF variables concerning railtypes Jan 15, 2019
@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch 2 times, most recently from 5b674bd to 0ae48df Jan 15, 2019
@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Jan 15, 2019

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.
New NML usage: "var[0x63, 0, 0xFF, ELRL]"

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)

src/newgrf_engine.cpp Outdated Show resolved Hide resolved
@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch from 0ae48df to 10c5c0c Jan 16, 2019
Copy link
Member

@LordAro LordAro left a comment

Looks fine, will wait for a GRF-God to approve & merge

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 21, 2019

We'll also want some docs once this is merged. Such admin :)

src/newgrf_engine.cpp Outdated Show resolved Hide resolved
@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch from 10c5c0c to 446d70f Jan 23, 2019
@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Jan 23, 2019

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

@LordAro LordAro requested a review from frosch123 Jan 27, 2019
@PeterN
Copy link
Member

@PeterN PeterN commented Feb 22, 2019

Hmm, why does a NewGRF need to check mappings? It should be transparent to it.

@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Feb 22, 2019

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.

@frosch123
Copy link
Member

@frosch123 frosch123 commented Feb 9, 2020

Last comment from a NewGRF author is over a year ago.
So there does not seem to be any demand for this.

@frosch123 frosch123 closed this Feb 9, 2020
@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Feb 11, 2020

uhm wtf kind of reasoning is that?!?

@EmperorJake
Copy link

@EmperorJake EmperorJake commented Sep 27, 2020

As a NewGRF Author, I demand this

@michicc michicc reopened this Sep 27, 2020
@michicc
Copy link
Member

@michicc michicc commented Dec 13, 2020

Needs a rebase. And I do think that it can improve handling of dual/multi-mode engines for NewGRFs.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Dec 13, 2020

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.

@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Dec 29, 2020

rebased, and removed the Var 3F part that was probably too hacky

@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch 3 times, most recently from 5279724 to c0e9cb7 Dec 29, 2020
@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Dec 29, 2020

also added range checks for Action 7

@michicc
Copy link
Member

@michicc michicc commented Dec 29, 2020

Principal approval from me.

@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch from c0e9cb7 to 82de515 Dec 30, 2020
@@ -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;

This comment has been minimized.

@frosch123

frosch123 Jan 2, 2021
Member

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.

src/newgrf_engine.cpp Outdated Show resolved Hide resolved
}
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;

This comment has been minimized.

@frosch123

frosch123 Jan 2, 2021
Member

This is not the negation of condition 0x13, which makes it weird in corner cases, and hard to implement in NML.

This comment has been minimized.

@Eddi-z

Eddi-z Jan 2, 2021
Author Contributor

that is intentional. in case one of the railtypes is undefined, both conditions return "false"

This comment has been minimized.

@Eddi-z

Eddi-z Jan 3, 2021
Author Contributor

maybe i'm too deep in formal logic here, but comparing <undefined> to <undefined> should never return true

This comment has been minimized.

@Eddi-z

Eddi-z Jan 3, 2021
Author Contributor

... 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;

This comment has been minimized.

@frosch123

frosch123 Jan 2, 2021
Member

Similar places issue a grfmsg() in this case. Though not sure whether this case always triggers in GRF loading stages before reservation.

This comment has been minimized.

@Eddi-z

Eddi-z Jan 3, 2021
Author Contributor

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.

@michicc
Copy link
Member

@michicc michicc commented Jan 3, 2021

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.

@Eddi-z
Copy link
Contributor Author

@Eddi-z Eddi-z commented Jan 3, 2021

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.

@michicc
Copy link
Member

@michicc michicc commented Jan 3, 2021

@Eddi-z After further though, yes. So that var should preferably also work for RVs.

@Eddi-z Eddi-z force-pushed the Eddi-z:var4A branch from 82de515 to 6899708 Jan 3, 2021
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;

This comment has been minimized.

@Eddi-z

Eddi-z Jan 3, 2021
Author Contributor

i'm suddenly unsure whether i should have used railtype_list instead here, like in the action7 changes

@frosch123
Copy link
Member

@frosch123 frosch123 commented Jan 10, 2021

I created a new PR #8554 to implement the ideas from this PR.
I salvaged a bit of the code of this PR, but most was unusable/broken.

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.

@frosch123 frosch123 closed this Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.