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

@andythenorth andythenorth commented Jul 20, 2018

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 label Sep 19, 2018
@andythenorth
Copy link
Contributor Author

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

@PeterN
Copy link
Member

@PeterN 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 nielsmh commented Jan 30, 2019

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

@PeterN
Copy link
Member

@PeterN 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 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 Jan 30, 2019
@PeterN
Copy link
Member

@PeterN 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 PeterN force-pushed the nrt-block branch 3 times, most recently from ac4f33c to 24b6afc Compare Feb 4, 2019
@PeterN PeterN dismissed nielsmh’s stale review Feb 7, 2019

Resolved

@PeterN PeterN force-pushed the nrt-block branch 9 times, most recently from ca85cd2 to e757d2d Compare Apr 19, 2019
@PeterN PeterN force-pushed the nrt-block branch 4 times, most recently from d978852 to 6c24853 Compare Apr 26, 2019
@PeterN
Copy link
Member

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

@stormcone stormcone commented Apr 30, 2019

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 PeterN commented May 1, 2019

@stormcone Thanks, this has now been fixed.

PeterN added 6 commits May 1, 2019
…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 approved these changes May 1, 2019
@michicc michicc merged commit 672c857 into OpenTTD:master May 1, 2019
8 checks passed
@Gymnasiast
Copy link

@Gymnasiast Gymnasiast commented May 1, 2019

Congratulations on getting this massive update finished! 🎉

stormcone added a commit to stormcone/nml that referenced this issue 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 issue 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 needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants