-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add: support for stations #244
Conversation
|
CHIPS port in progress to test this: https://github.com/andythenorth/chips/tree/nml-port |
I did some test trying to port CHIPS 'magic' stations.
|
Fixed the 2 issues I noticed (and reworked some part for the third time) |
Purchase/non-purchase splitting for callbacks may not be enough. Originally CHIPS defines 3 different chains for |
Applied #246. |
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.
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.
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.
Everything is done via var10, and all values (0-7) are used. Originally I only had "SPRITESET()" ("default" and "coal") and the direct reference in the layout. 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).
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
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
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.
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.
Woot! \o/ |
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.