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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add: support for stations #244

Merged
merged 12 commits into from Aug 29, 2022
Merged

Add: support for stations #244

merged 12 commits into from Aug 29, 2022

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Nov 1, 2021

It's finally possible to create stations newgrf with NML.
I implemented most props, but I'm not sure about their naming or simplicity of use. Maybe more global constants are needed.
I think I correctly implemented sprite layout for stations (was the hardest part).
I added a SPRITESET() function existing only for stations, used to refer to active spriteset (the one selected with basic action2).
Custom foundations are also implemented.
I tested many things locally and it works for me.

As an example, I converted CHIPS Cow pens station (and yes there are blinking cows 馃惍馃ぃ).
Regression test also produces a working grf.

@glx22
Copy link
Contributor Author

glx22 commented Nov 1, 2021

copy_sprite_layout can't work in the current implementation. I need to rethink things.

nml/actions/action3_callbacks.py Outdated Show resolved Hide resolved
nml/actions/action3_callbacks.py Outdated Show resolved Hide resolved
nml/actions/action0properties.py Show resolved Hide resolved
nml/global_constants.py Outdated Show resolved Hide resolved
@glx22 glx22 marked this pull request as ready for review November 6, 2021 20:20
@andythenorth
Copy link
Contributor

CHIPS port in progress to test this: https://github.com/andythenorth/chips/tree/nml-port

@glx22
Copy link
Contributor Author

glx22 commented Nov 17, 2021

I did some test trying to port CHIPS 'magic' stations.
Found some issues:

  • it's not possible to use LOAD_TEMP() in the layout, because the STORE_TEMP() will happen after registers are set (registers action2 is inserted in front of any action3 result)
  • some callbacks need a purchase specific version (at least select_sprite_layout)

@glx22
Copy link
Contributor Author

glx22 commented Nov 19, 2021

Fixed the 2 issues I noticed (and reworked some part for the third time)

@glx22
Copy link
Contributor Author

glx22 commented Nov 22, 2021

SPRITESET([offset]) renamed to DEFAULT([offset]).
Added CUSTOM(index, [offset]), with index pointing into custom_spritesets array.
Advanced test using most of the implemented stuff is available here.

Purchase/non-purchase splitting for callbacks may not be enough. Originally CHIPS defines 3 different chains for select_sprite_layout, 1 for default, 1 for purchase and 1 for cargos. prepare_layout may benefit from a similar spliting.

@glx22
Copy link
Contributor Author

glx22 commented Dec 6, 2021

Applied #246.

Copy link
Member

@frosch123 frosch123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed everything, understood little, but did not find anything wrong :)

I have 3 topics left.

Commit "Change: spritesets for stations don't need to have the same length"

I do not understand what this one is doing at all. If NML would support extended-action1, then the statement from the commit-message would hold true for all features. But this commit does not seem to be about extended-action1.

custom_spritesets, DEFAULT(), CUSTOM()

I understand the current behavior to be like:

  • DEFAULT() in the spritelayout references a spriteset resolved with var10=0. These sprites are choosen via switch()-chains, registered at "default" or cargo-types like "COAL".
  • CUSTOM() in the spritelayout references a spriteset resolved with var10=1. These sprites are choosen from a fixed list in "custom_spritesets", not using any switches or cargo types.

This asymmetry looks weird to me. How about:

  • Use "default" and "COAL" for all spritesets except foundations.
  • Replace "DEFAULT(i)" and "CUSTOM(i)" with a "SPRITESET(i, var10=0)".
  • Leave the switching on var10 for the user.

Tile layouts, pylons, wires, traversability

None of the examples use CB 24, so maybe noone needs this anyway.

The naming/constants are somewhat inconsistent to me:

  • Properties are named draw_pylon_tiles, hide_wire_tiles, non_traversable_tiles. These are nicely grouped with a "_tiles" postfix.
  • Callback 24 selects one of the 8 value-sets defined by these properties. Currently it is named custom_station_layout, which does not really describe what it does. I would expect this callback to reference the "_tiles" thingie, so maybe "select_tile_type".
  • There are tons of constants STAT_TILE_xxx for the results 0 to 7, I do not see anyone using them. IMO remove them.

nml/actions/action2var_variables.py Outdated Show resolved Hide resolved
@glx22
Copy link
Contributor Author

glx22 commented Aug 28, 2022

Reviewed everything, understood little, but did not find anything wrong :)

I have 3 topics left.

Commit "Change: spritesets for stations don't need to have the same length"

I do not understand what this one is doing at all. If NML would support extended-action1, then the statement from the commit-message would hold true for all features. But this commit does not seem to be about extended-action1.

Action2 SpriteLayout use spriteset from most recent action1, so all spritesets used by a layout must have the same number of sprites as they are in the same action1.
But for stations layouts, while sharing the same syntax, the only way to specify a spriteset is via var10, and that allows to use spritesets from different action1, and with different number of sprites.

custom_spritesets, DEFAULT(), CUSTOM()

I understand the current behavior to be like:

* DEFAULT() in the spritelayout references a spriteset resolved with var10=0. These sprites are choosen via switch()-chains, registered at "default" or cargo-types like "COAL".

* CUSTOM() in the spritelayout references a spriteset resolved with var10=1. These sprites are choosen from a fixed list in "custom_spritesets", not using any switches or cargo types.

This asymmetry looks weird to me. How about:

* Use "default" and "COAL" for all spritesets except foundations.

* Replace "DEFAULT(i)" and "CUSTOM(i)" with a "SPRITESET(i, var10=0)".

* Leave the switching on var10 for the user.

Everything is done via var10, and all values (0-7) are used.
For "default" and "coal" (using "DEFAULT(i)") var10 is not set, meaning it will be 0 as prop13 bit 0 is deliberately not exposed in NML.
For any other spriteset, directly referenced in the layout or via "CUSTOM()", a var10 value (1, 3-7) is assigned and mapped for it.

Originally I only had "SPRITESET()" ("default" and "coal") and the direct reference in the layout.
But then I though it would be nice to be able to define layouts with common parts (direct reference and/or "default"/"coal"), and item specific parts ("custom_spritesets" and "CUSTOM"), allowing to reuse layouts for different stations.
"custom_spritesets" can contain spritesets and switches (for dynamic spriteset selection).
https://gist.github.com/glx22/301ac434df8bfd41e137bc219a5bf465 heavily uses these possibilities, defining only 2 layouts with complex hide/show decision for 5 different stations. Each station using "CUSTOM(0)" for its specific ground type, and "CUSTOM(1)" for a dynamic choice of sprite to use as building.

Basically I tried to give user the choice to use very simple solution (many layouts with fixed spritesets + "default"/"coal") or highly dynamic (and complex).

Letting the user switch on var10 by itself might of course be doable (and a lot simpler implementation as it would remove all the internal var10 mapping). That would also means no direct spriteset references in the layout, only "SPRITESET()". I need to think about it (will probably try in another branch).

Tile layouts, pylons, wires, traversability

None of the examples use CB 24, so maybe noone needs this anyway.

The naming/constants are somewhat inconsistent to me:

* Properties are named `draw_pylon_tiles`, `hide_wire_tiles`, `non_traversable_tiles`. These are nicely grouped with a "_tiles" postfix.

* Callback 24 selects one of the 8 value-sets defined by these properties. Currently it is named `custom_station_layout`, which does not really describe what it does. I would expect this callback to reference the "_tiles" thingie, so maybe "select_tile_type".

* There are tons of constants `STAT_TILE_xxx` for the results 0 to 7, I do not see anyone using them. IMO remove them.

Done.

@@ -14,16 +14,14 @@
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA."""

from nml import expression, generic, nmlop
from nml.actions import action1, action2, action2var, action6, actionD, real_sprite
from nml.ast import general
from nml.actions import action0, action1, action2, action2real, action2var, action6, actionD, real_sprite

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [nml.actions.action2var](1) begins an import cycle.
from nml.actions import action1, action2, action2var, action6, actionD, real_sprite
from nml.ast import general
from nml.actions import action0, action1, action2, action2real, action2var, action6, actionD, real_sprite
from nml.ast import general, spriteblock

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [nml.ast.spriteblock](1) begins an import cycle.
Copy link
Member

@frosch123 frosch123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I did not quite understand the var10 usage from the diff.

Now I like it more than what I had in mind.

@glx22 glx22 merged commit fa5b89b into OpenTTD:master Aug 29, 2022
@glx22 glx22 deleted the stations branch August 29, 2022 12:52
@2TallTyler
Copy link
Member

Woot! \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants