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

Improvements to the Lua map generator #993

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@CelticMinstrel
Member

CelticMinstrel commented Apr 21, 2017

No description provided.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Apr 22, 2017

Member

Anyone have any idea why the latest commit doesn't work? There's two problems with it:

  1. If I use map:set_tile like the comment says it should, I get an assertion error saying that the map's width is broken (there are more or fewer terrains in some row than the map width).
  2. With the current implementation, the road is patchy and disconnected.
Member

CelticMinstrel commented Apr 22, 2017

Anyone have any idea why the latest commit doesn't work? There's two problems with it:

  1. If I use map:set_tile like the comment says it should, I get an assertion error saying that the map's width is broken (there are more or fewer terrains in some row than the map width).
  2. With the current implementation, the road is patchy and disconnected.

@Vultraz Vultraz added this to the 1.13.8 milestone Apr 27, 2017

@CelticMinstrel CelticMinstrel modified the milestones: 1.13.9, 1.13.8 May 12, 2017

@CelticMinstrel CelticMinstrel removed this from the 1.13.9 milestone Jun 13, 2017

@CelticMinstrel CelticMinstrel added this to the 1.14.1 milestone Jun 25, 2017

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 7, 2017

Contributor

@CelticMinstrel you have changed the milestone to 1.14.1 but since it has changes that add new syntaxes/features this cannot be done during a stable release. We could eigher make it before 1.14 or at 1.15 .

EDIT: since we already have some conability breaking changes in 1.13 with cave map generators i'd really prefer doiing this in 1.13 . If it's just the last commit that doesn't work we coudl alos merge the other commits already and postpone that one for 1.15

Contributor

gfgtdf commented Aug 7, 2017

@CelticMinstrel you have changed the milestone to 1.14.1 but since it has changes that add new syntaxes/features this cannot be done during a stable release. We could eigher make it before 1.14 or at 1.15 .

EDIT: since we already have some conability breaking changes in 1.13 with cave map generators i'd really prefer doiing this in 1.13 . If it's just the last commit that doesn't work we coudl alos merge the other commits already and postpone that one for 1.15

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 7, 2017

Member

@gfgtdf – Ultimately I wanted to have a multiplayer cave map which basically shows off the new cave generator features, and I believe the road cost thing is basically needed for that. I think the patchy and disconnected aspect of the road is probably something about order of operations (as if the road is being added correctly, but then something else is overwriting it later).

EDIT: Oh, the problem with using set_tile might be that it doesn't verify that the location is on the map.

Member

CelticMinstrel commented Aug 7, 2017

@gfgtdf – Ultimately I wanted to have a multiplayer cave map which basically shows off the new cave generator features, and I believe the road cost thing is basically needed for that. I think the patchy and disconnected aspect of the road is probably something about order of operations (as if the road is being added correctly, but then something else is overwriting it later).

EDIT: Oh, the problem with using set_tile might be that it doesn't verify that the location is on the map.

@CelticMinstrel CelticMinstrel modified the milestones: 1.13.9, 1.14.1 Aug 14, 2017

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 14, 2017

Member

Okay... there are still some things I want to do (mainly adding user config option on the cave map), but I think this could be mergeable at this point.

The road algorithm does seem like it could use some improvement, mind you - the road I'm getting is pretty patchy. Tweaks to the scenario WML for the cave map would also be welcome (maybe @sigurdfdragon would have some ideas on improving it?).

Member

CelticMinstrel commented Aug 14, 2017

Okay... there are still some things I want to do (mainly adding user config option on the cave map), but I think this could be mergeable at this point.

The road algorithm does seem like it could use some improvement, mind you - the road I'm getting is pretty patchy. Tweaks to the scenario WML for the cave map would also be welcome (maybe @sigurdfdragon would have some ideas on improving it?).

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Aug 15, 2017

Contributor

I may be able to take a look at this on Thursday.

Contributor

sigurdfdragon commented Aug 15, 2017

I may be able to take a look at this on Thursday.

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Aug 18, 2017

Contributor

@CelticMinstrel The things that may fix the patchiness are:

  1. There isn't a [road_cost] entry for all types of terrain that might be encounter. At least Sm (muddy swamp) and Rb^Fetd are listed as clear terrains in the generator, and there's no road cost them. Try adding those and looking for others. The gist is to make sure there is a [road_cost] tag for anything the generator may encounter from point A to B.

Also Ur^Ii & Ur need to have an entry at the bottom of the list of converting into themselves with a cost of 2

  1. Try more standard road costs for various terrains (ie, 10, 20, 25, 30 etc) Use 50 as the highest. Use 2 as the lowest amount. I don't know why / don't remember why mainline doesn't use 1 for MIN_ROAD_COST macros, but there's probably a reason.
Contributor

sigurdfdragon commented Aug 18, 2017

@CelticMinstrel The things that may fix the patchiness are:

  1. There isn't a [road_cost] entry for all types of terrain that might be encounter. At least Sm (muddy swamp) and Rb^Fetd are listed as clear terrains in the generator, and there's no road cost them. Try adding those and looking for others. The gist is to make sure there is a [road_cost] tag for anything the generator may encounter from point A to B.

Also Ur^Ii & Ur need to have an entry at the bottom of the list of converting into themselves with a cost of 2

  1. Try more standard road costs for various terrains (ie, 10, 20, 25, 30 etc) Use 50 as the highest. Use 2 as the lowest amount. I don't know why / don't remember why mainline doesn't use 1 for MIN_ROAD_COST macros, but there's probably a reason.
@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 20, 2017

Member

Thanks for the tips. I'll try applying these suggestions, probably tomorrow.

Member

CelticMinstrel commented Aug 20, 2017

Thanks for the tips. I'll try applying these suggestions, probably tomorrow.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 26, 2017

Member

Those tips seem to have worked, thanks! I think this can now be merged; I do still want to extend it to allow up to 9 players (or at least 8; 9 might be a bit hard), and to allow some configuration of parameters, but aside from that it all seems to be fully-functional, and the map it generates doesn't seem to be too terribly boring either. At most it could use maybe a few tweaks, maybe making the lake bigger, adding a river, or something like that.

Member

CelticMinstrel commented Aug 26, 2017

Those tips seem to have worked, thanks! I think this can now be merged; I do still want to extend it to allow up to 9 players (or at least 8; 9 might be a bit hard), and to allow some configuration of parameters, but aside from that it all seems to be fully-functional, and the map it generates doesn't seem to be too terribly boring either. At most it could use maybe a few tweaks, maybe making the lake bigger, adding a river, or something like that.

Allow mapgen user_config functions to mutate the [generator] config
(Also fixed a missing undef in the sample MP cave map)
@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 27, 2017

Member

...okay, I spoke a little too soon. There's this weird bug whereby the map (sometimes?) doesn't generate until you click "Regenerate". That is, at first you see nothing and it says the map is 0x0. Click "Regenerate", and it all works.

Member

CelticMinstrel commented Aug 27, 2017

...okay, I spoke a little too soon. There's this weird bug whereby the map (sometimes?) doesn't generate until you click "Regenerate". That is, at first you see nothing and it says the map is 0x0. Click "Regenerate", and it all works.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 27, 2017

Contributor

please make the return value of the lua user_config optional.

Contributor

gfgtdf commented Aug 27, 2017

please make the return value of the lua user_config optional.

@CelticMinstrel CelticMinstrel changed the title from [WIP] Improvements to the Lua map generator to Improvements to the Lua map generator Aug 27, 2017

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 27, 2017

Member

Okay, so basically all working now, I guess. There's still some room for improvement, mind you; if you make the lake large enough, you'll get villages dotted through it, for example. I also got twice as many castles as players, for some reason.

Member

CelticMinstrel commented Aug 27, 2017

Okay, so basically all working now, I guess. There's still some room for improvement, mind you; if you make the lake large enough, you'll get villages dotted through it, for example. I also got twice as many castles as players, for some reason.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 28, 2017

Member

In other words, unless @sigurdfdragon or others want to suggest some tweaks, this can be merged whenever.

Member

CelticMinstrel commented Aug 28, 2017

In other words, unless @sigurdfdragon or others want to suggest some tweaks, this can be merged whenever.

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Aug 28, 2017

Contributor

I'll be able to take another look at this on Wednesday.

Contributor

sigurdfdragon commented Aug 28, 2017

I'll be able to take another look at this on Wednesday.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 30, 2017

Contributor

just to be sure, this doesn't change behaviour of the 2 mainline scneraios where the cave random map generator is used, right?

Contributor

gfgtdf commented Aug 30, 2017

just to be sure, this doesn't change behaviour of the 2 mainline scneraios where the cave random map generator is used, right?

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 31, 2017

Member

It shouldn't, since it's only adding additional capabilities to the generator and using them in a brand-new MP scenario. Still, it wouldn't hurt to check, just to be sure...

(That said, I'd argue that we should consider using some of these new capabilities in the two mainline scenarios. With these upgrades it could be possible to get rid of all those SCATTER_EMBELLISHMENTS macro calls in the HTTT scenario, for example. However, that's outside of the scope of this PR.)

Member

CelticMinstrel commented Aug 31, 2017

It shouldn't, since it's only adding additional capabilities to the generator and using them in a brand-new MP scenario. Still, it wouldn't hurt to check, just to be sure...

(That said, I'd argue that we should consider using some of these new capabilities in the two mainline scenarios. With these upgrades it could be possible to get rid of all those SCATTER_EMBELLISHMENTS macro calls in the HTTT scenario, for example. However, that's outside of the scope of this PR.)

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Aug 31, 2017

Contributor

I found the following bugs:

  1. The number of villages produced on HttT 17 & SoF 4 is consistently lower than on current master. (ie, around 6-7 on SoF 4, whereas on master approx 15-35) The villages that are produced are at the bottom of the map, and the count thins out when you go up. This is most obvious on HttT 17
  2. The double keeps seem to only start occurring when the settings box is opened, regardless of wether or not anything is changed. Once it happens, it's sticks, until the create game screen has been exited and re-enterted.
  3. <illegal tile in map (2 Ko) '2 Ko'> (or similar) occurs occasionally on regenerating. The smaller the map & higher number of player seems to make it more likely to occur? My guess is that it might be trying to put a keep on top of an existing keep? (fixing double keep probably fixes this)
  4. 'Roads between castles' checkbox does not appear to work, as there are still roads when the checkbox is removed.
  5. Not sure if it is intentional or not, but the roads generated look to be a width of 3, whereas roads on the default map generator are typically a width of 1 hex.

Lake villages - A way of dealing with them could be to add a prestart event to convert villages surrounded by water into merfolk villages.

Contributor

sigurdfdragon commented Aug 31, 2017

I found the following bugs:

  1. The number of villages produced on HttT 17 & SoF 4 is consistently lower than on current master. (ie, around 6-7 on SoF 4, whereas on master approx 15-35) The villages that are produced are at the bottom of the map, and the count thins out when you go up. This is most obvious on HttT 17
  2. The double keeps seem to only start occurring when the settings box is opened, regardless of wether or not anything is changed. Once it happens, it's sticks, until the create game screen has been exited and re-enterted.
  3. <illegal tile in map (2 Ko) '2 Ko'> (or similar) occurs occasionally on regenerating. The smaller the map & higher number of player seems to make it more likely to occur? My guess is that it might be trying to put a keep on top of an existing keep? (fixing double keep probably fixes this)
  4. 'Roads between castles' checkbox does not appear to work, as there are still roads when the checkbox is removed.
  5. Not sure if it is intentional or not, but the roads generated look to be a width of 3, whereas roads on the default map generator are typically a width of 1 hex.

Lake villages - A way of dealing with them could be to add a prestart event to convert villages surrounded by water into merfolk villages.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Aug 31, 2017

Member

The roads were intentionally wide, with width=2... not sure why they're 3 wide though when I'm asking for 2 wide. I'll look into these issues, maybe this weekend or early next week.

Member

CelticMinstrel commented Aug 31, 2017

The roads were intentionally wide, with width=2... not sure why they're 3 wide though when I'm asking for 2 wide. I'll look into these issues, maybe this weekend or early next week.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Sep 6, 2017

Member

Do you still have issues with the settings dialog?

Member

Vultraz commented Sep 6, 2017

Do you still have issues with the settings dialog?

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Sep 6, 2017

Contributor

I should be able to check the settings issue post 86f0262

by the end of the week

Contributor

sigurdfdragon commented Sep 6, 2017

I should be able to check the settings issue post 86f0262

by the end of the week

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Sep 6, 2017

Member

Ok, I'm just asking since @CelticMinstrel mentioned some problems with the dialog and if those problems have to do with checkboxes then 5a05d7e might fix them.

Member

Vultraz commented Sep 6, 2017

Ok, I'm just asking since @CelticMinstrel mentioned some problems with the dialog and if those problems have to do with checkboxes then 5a05d7e might fix them.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Sep 7, 2017

Member

It's also possible that the roads_toggled function isn't correctly implemented, or the ignore key isn't correctly honoured - there are several possible points of failure.

Member

CelticMinstrel commented Sep 7, 2017

It's also possible that the roads_toggled function isn't correctly implemented, or the ignore key isn't correctly honoured - there are several possible points of failure.

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Sep 7, 2017

Contributor

Did a post 86f0262 test. It doesn't seem to help with any of the issues I mentioned above.

Contributor

sigurdfdragon commented Sep 7, 2017

Did a post 86f0262 test. It doesn't seem to help with any of the issues I mentioned above.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Sep 12, 2017

Contributor

i wonder whether we should enable wensoth.get_varibale in lua map generators so that the author can generate maps based on the variables set by the scenarios. Just a random idea though.

Contributor

gfgtdf commented Sep 12, 2017

i wonder whether we should enable wensoth.get_varibale in lua map generators so that the author can generate maps based on the variables set by the scenarios. Just a random idea though.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Sep 13, 2017

Member

That sounds like it'd be a lot of work, given that the WML context doesn't even exist yet when the generator runs. I suppose a way to fetch just the custom options might be useful, though.

Member

CelticMinstrel commented Sep 13, 2017

That sounds like it'd be a lot of work, given that the WML context doesn't even exist yet when the generator runs. I suppose a way to fetch just the custom options might be useful, though.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Feb 2, 2018

Member

pokes @CelticMinstrel to do something with this

Member

Vultraz commented Feb 2, 2018

pokes @CelticMinstrel to do something with this

@Vultraz Vultraz modified the milestones: 1.13.9, 1.14.0 Feb 2, 2018

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Feb 3, 2018

Member

Pretty sure I won't have time this weekend; any time I do have for Wesnoth will most likely be focused on the deprecation PR. I do want to get the material changes in this PR into master for 1.14, though, so if that's still an option after 1.13.11, maybe then? Obviously that would omit the new MP scenario (as that would violate the string freeze), but everything else could be merged

Member

CelticMinstrel commented Feb 3, 2018

Pretty sure I won't have time this weekend; any time I do have for Wesnoth will most likely be focused on the deprecation PR. I do want to get the material changes in this PR into master for 1.14, though, so if that's still an option after 1.13.11, maybe then? Obviously that would omit the new MP scenario (as that would violate the string freeze), but everything else could be merged

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Feb 11, 2018

Member

Postponed until 1.14.1.

Member

Vultraz commented Feb 11, 2018

Postponed until 1.14.1.

@Vultraz Vultraz modified the milestones: 1.14.0, 1.14.1 Feb 11, 2018

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Feb 14, 2018

Contributor

as noted eariler this cannot be done in a stable release, if it won't go into 1.14 it must be 1.15

Contributor

gfgtdf commented Feb 14, 2018

as noted eariler this cannot be done in a stable release, if it won't go into 1.14 it must be 1.15

@gfgtdf gfgtdf modified the milestones: 1.14.1, 1.15 Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment