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 commented Dec 29, 2018

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

@andythenorth
Copy link
Contributor

Tested, worked as expected for me.

@andythenorth
Copy link
Contributor

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

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

Catenary is defined by RTF_CATENARY.

@Eddi-z
Copy link
Contributor Author

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

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

@Eddi-z Eddi-z force-pushed the var4A branch 5 times, most recently from f01b3db to 2560ec4 Compare January 15, 2019 19:28
@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 var4A branch 2 times, most recently from 5b674bd to 0ae48df Compare January 15, 2019 20:24
@Eddi-z
Copy link
Contributor Author

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
LordAro
LordAro previously approved these changes Jan 20, 2019
Copy link
Member

@LordAro LordAro left a 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

@andythenorth
Copy link
Contributor

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
Copy link
Contributor Author

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

@PeterN
Copy link
Member

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

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 commented Feb 11, 2020

uhm wtf kind of reasoning is that?!?

@EmperorJake
Copy link
Contributor

As a NewGRF Author, I demand this

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

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

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.

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions needs review: NewGRF Review requested from a NewGRF expert size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
@Eddi-z
Copy link
Contributor Author

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 var4A branch 3 times, most recently from 5279724 to c0e9cb7 Compare December 29, 2020 16:37
@Eddi-z
Copy link
Contributor Author

Eddi-z commented Dec 29, 2020

also added range checks for Action 7

@michicc
Copy link
Member

michicc commented Dec 29, 2020

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;
Copy link
Member

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.

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;
Copy link
Member

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.

Copy link
Contributor Author

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"

Copy link
Contributor Author

@Eddi-z Eddi-z Jan 3, 2021

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

Copy link
Contributor Author

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;
Copy link
Member

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.

Copy link
Contributor Author

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.

@michicc
Copy link
Member

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 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 commented Jan 3, 2021

@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;
Copy link
Contributor Author

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

@frosch123
Copy link
Member

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
Labels
candidate: needs considering This Pull Request needs more opinions component: NewGRF This issue is related to NewGRFs needs review: NewGRF Review requested from a NewGRF expert size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants