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: Add NotRoadTypes (NRT) #6811

Merged
merged 6 commits into from May 1, 2019
Merged

Conversation

andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jun 3, 2018

The 'one big diff' approach.

16 types only, PeterN has a patch for 64, but that needs finish, tested, argued about etc. Can do that later. We make progress by not waiting for perfect eh? :)

Spec: https://wiki.openttd.org/Frosch/NotRoadTypes

Want to try it out? https://www.openttd.org/downloads/openttd-pullrequests/pr6811/latest.html has precompiled binaries for you! (last updated: 24th of March 2018)

@PeterN
Copy link
Member

PeterN commented Jun 3, 2018

Argument for doing 64 types initially is it saves having to add a bunch of savegame conversion code.

@andythenorth
Copy link
Contributor Author

Previous reviews by PeterN and TrueBrain took place in a ticket of the NRT fork (predated OpenTTD github).

andythenorth/NotRoadTypes#22 (comment)

@LordAro LordAro added the wip Work in progress. Feature branch that will require feedback during the development process label Sep 19, 2018
@andythenorth
Copy link
Contributor Author

andythenorth commented Jan 1, 2019

Rebased by Alberth using Peter's July 2018 rebase
https://github.com/Alberth289346/OpenTTD/tree/nrt-block-based

This builds and runs fine, but it's not clear yet:

  • if there are any compile warnings to attend to
  • whether docs (landscape_grid.html etc) are accurate
  • what state translation strings are in

With respect to 64 types (or not): this PR adds 64 types. It's the better option.

Crowd-sourced opinion is that this won't have enough play-test time to make it into 1.9.0 by April 2019. It has been play-tested quite a lot already, but eh. Save it for 2.0? 🎁

nielsmh
nielsmh previously requested changes Jan 5, 2019
Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

@PeterN
Copy link
Member

PeterN commented Jan 30, 2019

I have rebased to master, available here https://github.com/PeterN/OpenTTD/tree/nrt-block

New sprites have been split off to separate files to avoid future conflicts. I have not addressed the regression test failure.

@nielsmh
Copy link
Contributor

nielsmh commented Jan 30, 2019

The assertion failure mentioned above still happens in the regression tests.

@PeterN
Copy link
Member

PeterN commented Jan 30, 2019

The assertion failure mentioned above still happens in the regression tests.

Yes, #7142 has only just been merged into master!

@PeterN
Copy link
Member

PeterN commented Jan 30, 2019

Two issues I've noticed during gameplay testing:

  • Inconsistencies between roadtypes available and roadtypes buildable, resulting in misleading menus and button states. This may be something to do with date-introduced roadtypes.
  • Not possible to set up one-way roads.

@PeterN PeterN force-pushed the nrt-block branch 3 times, most recently from aada649 to 6fe4eee Compare January 30, 2019 23:43
@PeterN
Copy link
Member

PeterN commented Jan 30, 2019

One-way roads have now been fixed, this was due to the 64 road types addition. The regression failure is now resolved!

@PeterN
Copy link
Member

PeterN commented Apr 30, 2019

I guess I need to come clean on this, as it seems nobody knows: this is awaiting review and approval for merging.

@stormcone
Copy link
Contributor

If you try to "autoreplace" a ship or an aircraft, the game crashes:
"NOT_REACHED triggered at line 460 of OpenTTD-nrt-block/src/autoreplace_gui.cpp"

@PeterN
Copy link
Member

PeterN commented May 1, 2019

@stormcone Thanks, this has now been fixed.

PeterN added 6 commits May 1, 2019 18:28
…using rail.

INVALID_RAILTYPES, if it was accidentally tested, would match any railtype.
…ake sense.

Road type and rail type are stored in separate locations, so this parameter does
not make make sense as it is only used for rail bridges. Instead explicitly set the
rail type in MakeRailBridgeRamp().
@michicc michicc merged commit 672c857 into OpenTTD:master May 1, 2019
@Gymnasiast
Copy link

Congratulations on getting this massive update finished! 🎉

stormcone added a commit to stormcone/nml that referenced this pull request May 5, 2019
The number of GUI sprites has increased according to NRT (OpenTTD/OpenTTD#6811).
michicc pushed a commit to OpenTTD/nml that referenced this pull request May 5, 2019
The number of GUI sprites has increased according to NRT (OpenTTD/OpenTTD#6811).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: NewGRF This issue is related to NewGRFs needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants