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

Extended heightmap support #7664

Closed
wants to merge 83 commits into from
Closed

Conversation

ZornsLemma
Copy link

Hi,

Back in 2012 there was some discussion (at https://www.tt-forums.net/viewtopic.php?f=32&t=61140) about adding support for "extended heightmaps", which effectively allow a scenario to be defined as a series of bitmaps and textfiles and then loaded into OpenTTD to start a new game with, just as a PNG/BMP heightmap can be used at the moment. This has the advantage that the user gets to choose the NewGRFs they want to use instead of the scenario creator having to decided this, since they're just starting a new game as far as OpenTTD is concerned. Extended heightmaps also offer the possibility of the user choosing the size of map to play the "scenario" on, instead of this being baked in.

There's a draft spec for how this should work on the wiki: https://wiki.openttd.org/Terkhen/Scenario_format#Metadata_file

As discussed recently in the thread linked above, I've taken the existing patches (mostly by Terkhen, I think) and extended them to support loading an extended heightmap with height and town layers (the full spec has many different layer types).

I'd appreciate it if someone could do a brief review of the code on my branch and let me have any feedback, be it on minor problems or things which are just "not done in the OpenTTD way". There are a few particularly iffy points noted via comments containing the string 'EHTODO' which would probably benefit most from some expert attention. I'd like to avoid adding more code to support other features of the spec if I'm doing this in the wrong way!

The individual commits on my branch are just me talking to myself as I developed the code; anyone interested in doing a review should almost certainly just diff my branch against master.

Please let me know if you have any questions, of course!

Cheers.

Steve

heightmap_type.h should actually have been added in the previous
commit...
I checked the code for FixSlopes() and FlatEmptyWorld() and it hasn't
changed since this patch was created, so this is just moving the current
code and not throwing away any recent bug fixes.
It now builds correctly.
Without these changes:
- heightmaps with nearly-zero-but-not-quite greyscales could end up with
  low-lying land replaced by sea (compared to the behaviour without the
  extended heightmap patches; we need to be compatible for legacy
  heightmaps)
- the "maximum map height" specified when loading a legacy heightmap was
  not respected, because LoadLegacyHeightmap() copied it into
  ExtendedHeightmap before the dialog gave the user a chance to edit it.
I think we are now able to take the rotation from the metadata, show it
to the user as the default but allowe the user to override it.
We now seem to be correctly parsing width/height/orientation from
vg[12].ehm
max_height isn't "wrong" (although it is given the test bitmaps go up to
greyscale 255) but I wanted to express max_desired_height. It's not
acted on yet, but will be soon.
Wiki says to do this instead of erroring as we did previously.
@nielsmh
Copy link
Contributor

nielsmh commented Jul 21, 2019

Some general comments:

  • The commit history needs to be squashed at some point, doesn't have to be right away. I don't think it's a good idea to add this in a single huge commit, better to make it several logical commits.
  • I haven't figured out yet if the stuff under eh-test/ is proper automated test data, or just for manual testing, but at the very least it needs to be moved to somewhere else in the tree.
  • Overall the code looks pretty good.

@nielsmh
Copy link
Contributor

nielsmh commented Jul 21, 2019

The build fails on Windows, probably due to some differences in STL structure. Some includes seem to be missing.
Also one of the things it complains about is auto_ptr, you should probably use unique_ptr instead.

The build on Mac also fails due to a problem with STL includes.

The commit checker isn't going to be happy until the commit messages have been changed.

@ZornsLemma
Copy link
Author

Thanks very much for taking a look!

Could you please let me have the failed build output for Windows? I don't have a Windows dev environment I can use but if I can see what went wrong I might be able to work out what the problem is. I'd hope if I can fix that then there's a chance of it building on the Mac too.

I did try using unique_ptr but it failed to build (on Linux, which is what I'm doing my development on); I think g++ only enables it if building with -std=c++14, not -std=c++11 as OpenTTD seems to use. My inclination is to stick with auto_ptr for now and rewrite the code to use raw pointers once it's otherwise finished; auto_ptr is really just a minor convenience as I'm using it here to avoid needing to remember to delete the memory at every early return.

The stuff under eh-test is just manual test data for my convenience and to serve as examples for anyone who wants to play with this; I want that in git for now but it would be removed before submitting any final pull request.

I'm happy to rework the commit history before eventually submitting a final pull request; at the moment I was just looking for the kind of feedback you've given before pushing on with the implementation of some more layers and see how that goes. Thanks once again for taking a look, I appreciate it!

@michicc
Copy link
Member

michicc commented Jul 21, 2019

OTTD is already using unique_ptr, even on Linux, so I'd guess you made an error somewhere. auto_ptr should really not be used due to the bad semantics of it.

The output of all CI checks can be viewed with the Details link.

@ZornsLemma
Copy link
Author

You're right; the problem is that I was trying to use std::make_unique. I've pushed a commit which changes auto_ptr to unique_ptr, cheers!

I'll take a look at the CI checks. I didn't realise there would be an automated build, I thought nielsmh had tried to build it himself.

@Eddi-z
Copy link
Contributor

Eddi-z commented Jul 21, 2019

It's actually a bit buried. You need to click on "details" in the CI build, but that output is horrible. then scroll to the bottom there and click on "view more details", then you can click on the platform you wish to check, and there it has the full output log

I hope this will fix the build on Mac...
@andythenorth
Copy link
Contributor

andythenorth commented Nov 2, 2019

Thanks for this! Extended heightmaps is something that's wanted, so it's good to see this PR.

However there has been no progress for 3 months, and it's failing the automated checks. I do close some older PRs if they have no progress, as it keeps the project healthy. I'll close this one in a month or so if nothing changes. Thanks!

@andythenorth andythenorth added the stale Stale issues label Nov 2, 2019
@ZornsLemma
Copy link
Author

Thanks, please do feel free to close it straight away if that's better for you. I do intend to pick this up again at some point in the future, but it's unlikely to be within the next month. (And if anyone else is reading this, do feel free to do build on what I've done here.)

@minexew
Copy link

minexew commented Apr 22, 2020

With this patchset, is it possible to create extended heightmaps from SAV/SCN?

If not, is this something that should be worked on?

@ZornsLemma
Copy link
Author

This work is not complete - this was just a draft pull request to get some initial feedback from the OTTD devs. At the moment it only allows loading of extended heightmaps (and a very limited form of them at that). Ultimately I would like to see it extended to allow saving of extended heightmaps, and at that point you would then be able to load a SAV/SCN and save it out as an extended heightmap, but that functionality doesn't exist yet. There are some patches on the forum thread which would probably be helpful in supporting this, but they're not complete.

I am not personally working on this at the moment - I may pick it up again in the future, of course. My work was built on some other people's earlier incomplete efforts, and if anyone feels keen to pick this up before I come back to it please do. (It would probably be good to just post a note on the forum thread to avoid duplicating effort.)

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed and removed stale Stale issues labels Dec 14, 2020
@TrueBrain
Copy link
Member

Hi @ZornsLemma ! Hopefully you got the feedback you were looking for! As you pointed out yourself, you are not actively working on this Pull Request, I am going to close it for now. At any point feel free to reopen it, and if you want us to challenge your ideas or implementation, just let us know!

Cheers!

@TrueBrain TrueBrain closed this Dec 21, 2020
@ZornsLemma
Copy link
Author

Thanks, that makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants