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

Add new mushroom base terrain and make the overlay have mixed defense #4299

Merged
merged 1 commit into from Sep 11, 2019

Conversation

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Aug 31, 2019

Addresses #4213 / #4214 since no-one else seemed interested in doing it. Graphics courtesy of @doofus-01.

Probably needs work on the transitions. I started with the original (now-reverted) commit and tweaked it to create new terrains instead of modifying the old terrain. Also, if anyone has a better terrain code than Tb, please suggest it.

I didn't fiddle with any existing maps, as I'm not sure whether it would be preferred to replace *^Uf with Tb or with *^Tf. That should be left to someone who cares about MP balance.

@CelticMinstrel CelticMinstrel changed the title [WIP] Add new mushroom base terrain and make the overlay have mixed defense Add new mushroom base terrain and make the overlay have mixed defense Aug 31, 2019
@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented Aug 31, 2019

Do you have a screenshot of the before and after? What is this change supposed to look like?

@@ -2721,7 +2758,7 @@ For those who go by land or sea, a bridge is the best of both worlds — for gam
id=fungus
name= _ "Fungus"
editor_name= _ "Fungus"
string=Uft
string=Tt

This comment has been minimized.

Copy link
@jostephd

jostephd Aug 31, 2019

Member

This seems to undefine the Uft terrain code. Is that so? Is there a backward compatibility problem here?

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Aug 31, 2019

Author Member

The Uft code isn't a code used on maps, it's the "archetype" used only as a base for other terrains and to provide general information about that archetype (like the sidebar icon and help text). As you can see, it's marked hidden, and furthermore it doesn't actually have any graphics, so unless someone manually edited it into their map, there shouldn't be any issues.

This comment has been minimized.

Copy link
@stevecotton

stevecotton Aug 31, 2019

Contributor

If someone's cut&pasted terrain definitions in to their UMC, then those terrains might be using Uft as a base. In 1.14, it seems that all terrain aliases use only the virtual terrains as their bases, and I think the help pages start doing odd things if you don't (as in, the game creates a help section for every terrain used as a base, even if it's not a virtual terrain).

Edit: OTOH, that would be something that the UMC author could easily update when porting to 1.15.

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Sep 11, 2019

Author Member

And wmllint could add rules to auto-update terrain definitions that alias to Uft,

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Aug 31, 2019

It's not supposed to change appearance of any existing terrains. I'll grab a screenshot at some point, but probably tomorrow as it's late and I should sleep.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Aug 31, 2019

Thanks for working on this. I do have a screenshot, but it doesn't look quite right:

2019-08-31-035237_278x267_scrot

Pressing Ctrl+T on the first row opens the help at the homepage.

Pressing Ctrl+T on the second row opens the terrain help page, the terrain image shows mushrooms on grass. It's a mixed terrain.

Pressing Ctrl+T on the third row opens a more generic terrain description page, there's no image nor a list of stats, it's not a mixed terrain.

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented Aug 31, 2019

I don't personally use mixed mushroom terrains for their unit movement/defense properties, so balancing around the new behavior isn't going to be an issue for me at least. Since it doesn't change the appearance of existing terrains, I'm totally fine with this.

Edit: didn't see josteph's screenshot till now, probably this will want a bugfix first.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Aug 31, 2019

Well, that's why I marked it WIP. I'll take a look at the issue tomorrow.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Aug 31, 2019

Since it doesn't change the appearance of existing terrains, I'm totally fine with this.

If it doesn't change the appearance of existence terrains, it shouldn't claim to address #4214.

@CelticMinstrel CelticMinstrel force-pushed the mushrooms branch from a423996 to 8afbb5c Aug 31, 2019
@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Aug 31, 2019

It addresses #4214 by superceding it in my view? But if you disagree, then whatever.

Anyway, looks like it works now. A lot of the problems were caused by conflicting terrain IDs (oops!).

@CelticMinstrel CelticMinstrel marked this pull request as ready for review Aug 31, 2019
@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Aug 31, 2019

It addresses #4214 by superceding it in my view? But if you disagree, then whatever.

I'm not trying to nitpick. #4214 is about changing the graphics of ^Uf on master to match its stats. This PR doesn't change either the graphics or the stats of ^Uf, so it doesn't fix #4214. I was just trying to clarify this point. Let's move on.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Sep 1, 2019

One minor detail I was wondering is whether the default base of the new mushrooms should be Uu (as with the old mushrooms) or Tb.

@Vultraz

This comment has been minimized.

Copy link
Member

Vultraz commented Sep 1, 2019

Honestly, I do not find this worth it at all. Like I said on Discord (just copying it), in order to solve one terrain not functioning quite as expected, we're proposing deprecating the whole terrain and introducing a new one with a full set of graphics, necessitating all mainline and UMC maps that use the mushrooms (including in non-cave areas!) be updated. The new terrain proposed, with it's cave floor base, doesn't work in outdoor maps.

This is not just a graphics change, like when we went from smooth cave walls to mine walls. That change did result in some aesthetic differences but the general purpose was the same. This changes the use of the terrain completely.

And what do we get out of it? Sure, the terrain becomes less confusing and more consistent. But the nominal gain for the amount of work just does not seem worth it. At all.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Sep 1, 2019

@Vultraz

The new terrain proposed, with it's cave floor base, doesn't work in outdoor maps.

Uh. What the heck are you talking about? This statement seems to be in disagreement with reality...

  1. It doesn't have a cave floor base, it has a mushroom floor base.
  2. You can put it on whatever base you want.
  3. Exactly why wouldn't it work in outdoor maps?
@Vultraz

This comment has been minimized.

Copy link
Member

Vultraz commented Sep 1, 2019

I meant now you can't put it on top of hills, mountains, swamp, etc.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Sep 1, 2019

Uh... yes you can?

@stevecotton

This comment has been minimized.

Copy link
Contributor

stevecotton commented Sep 1, 2019

One minor detail I was wondering is whether the default base of the new mushrooms should be Uu (as with the old mushrooms) or Tb.

I think Tb - that way, when updating maps that have ^Uf terrains on them:

  • left-click places terrain which is functionally the same
  • shift-left-click places terrain which is cosmetically the same

I'm currently playing through Eastern Invasion, updating the maps.

@Vultraz

This comment has been minimized.

Copy link
Member

Vultraz commented Sep 1, 2019

Is it still the same giant mushrooms with a new optional base?

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Sep 1, 2019

There are three new terrains in this PR:

  1. Tb, which is a base terrain with mushroom properties
  2. ^Tf, which is visually equivalent to ^Uf and aliases to mushrooms, but has mixed properties like forest.
  3. ^Tfi, which is the illuminated version of above, like ^Ufi.
Copy link
Contributor

stevecotton left a comment

I think we should merge this, and then it's the baseline for any follow-up PRs with improvements. By which I'm thinking of:

  • Updating the mainline campaigns (I have updates for EI and UtBS ready to push)
  • Making the old vs new mushrooms easy to see in the editor
@Pentarctagon

This comment has been minimized.

Copy link
Member

Pentarctagon commented Sep 6, 2019

Sounds good to me as well, FWIW.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Sep 11, 2019

Oh, I just (re-)noticed @jostephd's observation about Ctrl+T... I never thought to test that myself. Did you ever check up on it again after I fixed all the other issues?

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Sep 11, 2019

Oh, I just (re-)noticed @jostephd's observation about Ctrl+T... I never thought to test that myself. Did you ever check up on it again after I fixed all the other issues?

I did not test it again.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

CelticMinstrel commented Sep 11, 2019

Okay, I just checked... Ctrl+T works for the overlay and shows the correct topic depending on the base terrain, but for some reason it doesn't work for the base terrain, even if I put a different overlay on it.

I don't think it's a blocker though.

@CelticMinstrel CelticMinstrel merged commit d9cebd6 into master Sep 11, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.