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

Feature: [NewGRF] Allow higher max speeds for ships #10734

Merged
merged 1 commit into from
Jan 28, 2024

Conversation

Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Apr 28, 2023

Motivation / Problem

I recently made this fix: #10695 . In the original bug report was a link to a forum thread about the Ekranoplans https://www.tt-forums.net/viewtopic.php?t=39188 . These are Soviet era ground effect ships that can go up to 500 km/h, but the max speed of ships in openttd is 128 km/h. This PR changes that.

Description

I added a new uint16 value that allows higher ship speeds to be set. I also added a byte to set the acceleration, because with the default acceleration fast ships take a very long time to accelerate, which looks a little odd.

Limitations

More a limitation of the dev: this is the first time I've worked on anything NewGRF-related. Please let me know if I forgot something.

I've never created a NewGRF and obviously there are no NewGRFs available that contain these new values. For testing I just hacked the code in newgrf.cpp:1596 to multiply the ship speed by e.g. 4, and set the acceleration value to a high value. It would be great if somebody with a bit of newgrf-making experience could verify if it works.

NOTE TO SELF: https://newgrf-specs.tt-wiki.net/wiki/Action0/Vehicles/Ships update this page when this PR is merged.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@PeterN
Copy link
Member

PeterN commented Apr 28, 2023

Not sure that how this was done for Road Vehicles is a good example... nor do I know why it was done that way.

Just adding a uint16 to set max_speed seems to make more sense to me.

@Kuhnovic Kuhnovic force-pushed the Higher_max_ship_speed branch 2 times, most recently from 2414ab4 to aa33bec Compare April 30, 2023 20:53
@rubidium42
Copy link
Contributor

Not sure that how this was done for Road Vehicles is a good example... nor do I know why it was done that way.

Just adding a uint16 to set max_speed seems to make more sense to me.

I agree that setting the maximum speed as a new uint16 property, just like the long format introduction (1A vs 00 for ships), would be better for the future. Similarly, why not expose the acceleration of the vehicle to NewGRFs as well? Aircraft has such a thing, though maybe use a more sensible unit?

@Kuhnovic
Copy link
Contributor Author

Kuhnovic commented May 5, 2023

@PeterN @rubidium42 The property is now an uint16, and the acceleration is also added as a property. The acceleration is 0.5 km/h per tick, which is still not a really sensible unit. These numbers are basically dictated by the integer math that control ships, not much I can do about it without more high-impact changes.

src/newgrf_properties.h Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler added the needs review: NewGRF Review requested from a NewGRF expert label Sep 7, 2023
@2TallTyler
Copy link
Member

I'd be interested in seeing this in 14.0 alongside the new ship pathfinder, if we have a qualified reviewer (which I am not) to sign off on it.

@2TallTyler 2TallTyler added this to the 14.0 milestone Dec 23, 2023
@rubidium42
Copy link
Contributor

I've resolved the merge conflict and reverted the change in the value of PROP_SHIP_SPEED as that value is essentially defined via the NewGRF callback specification. I haven't tested it though.

@2TallTyler
Copy link
Member

@zephyris Would you be interested in dusting off your old Ekranoplan GRF to test this? (or making the source available for @Kuhnovic to test 🙂)

I've added this to the 14.0 milestone so hopefully it doesn't slip through the cracks, but somebody has to test it and say if it works and makes sense. 😉

@zephyris
Copy link
Contributor

Yeah, of course. I'll have to do some digging though. The code may have been lost to an openttdcoop hole in the cloud, but I should have all original sprites and reverse engineering the code should be easy enough.

I've just moved house, so no Internet right now, feel free to remind me in a week or so if no progress.

@zephyris
Copy link
Contributor

zephyris commented Jan 23, 2024

ekranoplan.nfo, last modified 2008/08/31(!), looks like we're in luck!

@zephyris
Copy link
Contributor

Ok, I've got my ekranoplan grf essentially revived. I haven't got a nice nfo source as it's essentially just a demo grf with no reasonable stats, so if you want an nfo file to work from then it's best to just decompile the grf file here: https://www.tt-forums.net/viewtopic.php?p=723787#p723787

I've also grabbed the sprites and set them up for a nicer nml source. Currently skeletal vehicle stats, but nicely templated and commented. I'll polish this and put it up on my openttd-newgrfs repo at some point - but let me know if you'd like a copy to work on in the meantime.

@zephyris
Copy link
Contributor

Another day, another bump. I've finished NML recode of the ekranoplan newGRF, available here: https://github.com/zephyris/openttd-newgrfs/tree/main/ekranoplan

Currently, their speeds are all 127 km/h, but there are comments with the correct speed for each vehicle.

@zephyris
Copy link
Contributor

ekranoplan.zip
This is a test grf, coding a late-game ekranoplan (intro year 2029). There's two versions, one coded with 0B FE (127 km/h) for speed and one with 23 14 05 (650 km/h).

@rubidium42
Copy link
Contributor

This is a test grf, coding a late-game ekranoplan (intro year 2029). There's two versions, one coded with 0B FE (127 km/h) for speed and one with 23 14 05 (650 km/h).

ekro

The NewGRF works! Only caveat is that it needs almost 256 tiles to get up to speed with the slow acceleration it got now (not configured). Ship 1 is the "slow" one, Ship 2 is the "fast" one.

@zephyris
Copy link
Contributor

Awesome. This is a test newGRF with three variants, one now with 23 14 05 and 24 FE for high speed and super-high acceleration. ekranoplan2.zip

Assuming this works as expected, this looks decent from a newGRF author perspective. Just one query: What's the default acceleration? ie. default value for 0x24. Is it 1? That doesn't allow slower acceleration than default ships, not that I think that slower acceleration would realistically ever be needed.

@TrueBrain TrueBrain changed the title Higher max ship speed Feature: [NewGRF] Allow higher max speeds for ships Jan 27, 2024
@rubidium42
Copy link
Contributor

The super-high acceleration is really noticeable.
Roughly speaking for a ~250 long route, one leg of the "slow" one equals 3 legs of the fast one with slow acceleration and equals 5 legs of the fast one with fast acceleration.

For slower acceleration we would need to get fractional speeds for ships. That's going to be a huge hassle, so I do not think making acceleration any slower would be a good idea.

@Kuhnovic
Copy link
Contributor Author

The default acceleration is indeed 1 unit per tick, in some kind of bizarre unit that I can't remember. IMO we don't need any slower than that, but we do need higher acceleration for fast ships to look a bit more realistic.

@zephyris you might have overdone the acceleration in your newgrf a bit though :P . It is a lot of fun to play around with!

@zephyris
Copy link
Contributor

That was deliberate! I wanted to check that a single byte for acceleration was appropriate. Back of the envelope calculation says yes, but seeing it in-game is the real test.

Just need some nml support now, so I don't have to hack through decompiled nfo to get the high speeds set :)

Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Code looks good to me, and Ekranoplan seems like an excellent stress-test to prove that it works for NewGRF authors. 😃

@rubidium42 rubidium42 merged commit 5a55c4a into OpenTTD:master Jan 28, 2024
20 checks passed
@Kuhnovic Kuhnovic deleted the Higher_max_ship_speed branch February 7, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review: NewGRF Review requested from a NewGRF expert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants