HomeWildfire Games

Reveal the map on the wall demo map (so that it isn't entirely nor partially…

Description

Reveal the map on the wall demo map (so that it isn't entirely nor partially invisible independent of the selected gamemode and ownership).

Fixes #4535
Reviewed By: sanderd17

Event Timeline

Sandarac added inline comments.
/ps/trunk/binaries/data/mods/public/maps/random/wall_demo.json
15

Duplicate newline.

/ps/trunk/binaries/data/mods/public/maps/random/wall_demo_triggers.js
1

Not sure about adding this entire file just for this one line to reveal the map...

elexis added inline comments.Apr 23 2017, 10:35 PM
/ps/trunk/binaries/data/mods/public/maps/random/wall_demo.json
15

Lesson learnt: gedit hides the last newline

leper raised a concern with this commit.Apr 23 2017, 11:39 PM
leper added a subscriber: leper.

I guess removing those "XXXX": "Add properties like you would for other maps" things (or roughly that wording) resulted in people not considering just doing that.

/ps/trunk/binaries/data/mods/public/maps/random/wall_demo.json
10

"RevealMap": true

This commit now has outstanding concerns.Apr 23 2017, 11:39 PM

I guess removing those "XXXX": "Add properties like you would for other maps" things (or roughly that wording) resulted in people not considering just doing that.

rP17981

"XXXXXX" : "Optionally define other things here, like we would for a scenario"

The hint states that one can add properties, but not which ones and therefore might be confused with map-specific properties.
That we can add properties to an object is self-evident, but mentioned in the string.
Which properties can be defined (those from the gamesetup) and that we can also define custom map-specific ones isn't, but not mentioned in the string.
Documentation shouldn't be duplicated once per map.
Documentation shouldn't be stored in a simulation variable.
RevealMap is present in 139 maps in my svn folder.

The proposal to use the property doesn't work, because the user can (and is supposed to be able to) control the gamesetup settings on random maps. If the the map revelation option is disabled, it will start invisible.

I guess removing those "XXXX": "Add properties like you would for other maps" things (or roughly that wording) resulted in people not considering just doing that.

rP17981

"XXXXXX" : "Optionally define other things here, like we would for a scenario"

That's the one

The hint states that one can add properties, but not which ones and therefore might be confused with map-specific properties.

They could be ;-)

[...]

Yes, having that pointless property is just that. I was mostly trying to be funny.

RevealMap is present in 139 maps in my svn folder.

So making that 140 (and introducing that fancy thing to random maps) shouldn't be that bad.

The proposal to use the property doesn't work, because the user can (and is supposed to be able to) control the gamesetup settings on random maps. If the the map revelation option is disabled, it will start invisible.

Yes, players selecting stupid things will result in stupid things happening. But this is a demo map, so having a sane default should be enough.

I just object to not going with the easy solution and instead adding another file (but I do object that one Units Demo change that destroyed the nice single file property of that map (and that one can have such custom code in there if one wants to)).

I just object to not going with the easy solution and instead adding another file (but I do object that one Units Demo change that destroyed the nice single file property of that map (and that one can have such custom code in there if one wants to)).

Agree that avoiding the file would be ideal. We also considered changing the owner of the walls to 1, but then it won't work if the assigned player happens to be > 1 (and since it's a random map we can't force the playercount).
Also considered insta-defeating the player (i.e. we discovered a Conquest bug), but that won't work with victory conditions that don't inherit Conquest. Also we actually want the entire map to be revealed.

When setting the JSON flag, it will indeed set it as default.
But it also has the side-effect that this setting will be persisted (independent of that infamous config option) when selecting other maps.
So if one has set the "all maps" filter and then scrolls through the maps to find a suitable one, the map will become revealed without the user noticing.
(See also https://code.wildfiregames.com/D232#8651 )

Agree that avoiding the file would be ideal. We also considered changing the owner of the walls to 1, but then it won't work if the assigned player happens to be > 1 (and since it's a random map we can't force the playercount).

Then we would have to deal with the structures decaying, which just delays the issue.

Also considered insta-defeating the player (i.e. we discovered a Conquest bug), but that won't work with victory conditions that don't inherit Conquest. Also we actually want the entire map to be revealed.

The map should be of type endless. Is that bug that we don't check whether the victory condition holds on game start? (If yes, we might want to do that only after the first turn (else we could run into issues with skirmish replacements).)

But it also has the side-effect that this setting will be persisted (independent of that infamous config option) when selecting other maps.

That should be something that the gamesetup rewrite fixes. Also we should really nuke (or fix) that option.

So if one has set the "all maps" filter and then scrolls through the maps to find a suitable one, the map will become revealed without the user noticing.

Should we really show demo maps in that case? Yes, that would make that "all" part a blatant lie, but those are just useful for testing (or when someone makes a new map in atlas and forgets to tick (or untick) a checkbox).

(See also https://code.wildfiregames.com/D232#8651 )

I do not consider the RevealMap setting even similar to that one. But a sane gamesetup should be able to handle both nicely.

Also considered insta-defeating the player (i.e. we discovered a Conquest bug), but that won't work with victory conditions that don't inherit Conquest. Also we actually want the entire map to be revealed.

The map should be of type endless. Is that bug that we don't check whether the victory condition holds on game start? (If yes, we might want to do that only after the first turn (else we could run into issues with skirmish replacements).)

It is

the gamesetup rewrite

(Singular is not really appropriate in that context. I won't add more complexity to D322, there is still a rewrite needed to fix the infamous config option (we could only save the chosen settings and derive g_GameAttributes and not save the latter to a file nor broadcasting it via network).
Adding the logic to have options that only work on some maps will likely end up in another rewrite.)

So if one has set the "all maps" filter and then scrolls through the maps to find a suitable one, the map will become revealed without the user noticing.

Should we really show demo maps in that case? Yes, that would make that "all" part a blatant lie, but those are just useful for testing (or when someone makes a new map in atlas and forgets to tick (or untick) a checkbox).

This workaround will work around the bug in this case, but next time but we will end up in the same situation next time we have a similar issue with a non-demo map.

That should be something that the gamesetup rewrite fixes

(See also https://code.wildfiregames.com/D232#8651 )

I do not consider the RevealMap setting even similar to that one.

Doesn't matter if it's population capacity, map revelation or any other gamesetting. The design goal has been and still is that the user can chose any setting on random maps and change the map without getting his configuration changed after changing the map.

elexis requested verification of this commit.Apr 26 2017, 6:09 PM
This commit now requires verification by auditors.Apr 26 2017, 6:09 PM
leper raised a concern with this commit.Apr 26 2017, 6:48 PM

This workaround will work around the bug in this case, but next time but we will end up in the same situation next time we have a similar issue with a non-demo map.

I'm not sure if we should care about revealed non-demo maps. And if we consider the user changing things and keeping that to be important then we should also regard the user doing stupid things and getting such results to be a consequence of that.

That should be something that the gamesetup rewrite fixes

(See also https://code.wildfiregames.com/D232#8651 )

I do not consider the RevealMap setting even similar to that one.

Doesn't matter if it's population capacity, map revelation or any other gamesetting. The design goal has been and still is that the user can chose any setting on random maps and change the map without getting his configuration changed after changing the map.

Then we need to remember if some setting was changed explicitly.

This commit now has outstanding concerns.Apr 26 2017, 6:48 PM

Doesn't matter if it's population capacity, map revelation or any other gamesetting. The design goal has been and still is that the user can chose any setting on random maps and change the map without getting his configuration changed after changing the map.

Then we need to remember if some setting was changed explicitly.

The user expects settings to remain the same when selecting a different random map. This is not related to whether or not the user has set a setting explicitlyo r whether it was the default and the user intended to keep it that.

This workaround will work around the bug in this case, but next time but we will end up in the same situation next time we have a similar issue with a non-demo map.

I'm not sure if we should care about revealed non-demo maps.

Don't understand this comment

And if we consider the user changing things and keeping that to be important then we should also regard the user doing stupid things and getting such results to be a consequence of that.

The code is working perfectly fine and meets all criteria. Don't see the point of raising a concern. If there is a patch that changes settings behind the users back, that would be a good reason to raise a concern.

Doesn't matter if it's population capacity, map revelation or any other gamesetting. The design goal has been and still is that the user can chose any setting on random maps and change the map without getting his configuration changed after changing the map.

Then we need to remember if some setting was changed explicitly.

The user expects settings to remain the same when selecting a different random map. This is not related to whether or not the user has set a setting explicitlyo r whether it was the default and the user intended to keep it that.

I'd expect settings I set to remain that way, and pick whatever the map has for the rest. Or should we load some trigger scripts on other random maps just because one of the previously selected ones did? (Not talking about victory conditions here.)

And if we consider the user changing things and keeping that to be important then we should also regard the user doing stupid things and getting such results to be a consequence of that.

The code is working perfectly fine and meets all criteria. Don't see the point of raising a concern. If there is a patch that changes settings behind the users back, that would be a good reason to raise a concern.

Maybe I just fail to see the point why we need to add a new file when a simple setting could do the same.

@mimo @Imarok @bb can you answer the following questions?

  • Do you observe a different player behavior with random maps in comparison to scenario and skirmish maps?
  • Is it a good use case to be able to browse through existing random maps without having the user settings changed?
  • Does it happen to be a reason that many players only play random maps?
  • Should we drop all map settings upon map selection for random maps like we do for scenario and skirmish maps?
  • Or should we show a chat notification "The map is now revealed" to inform players that a setting was changed behind their back to prevent starting a match with unintended settings after possibly half an hour of gamesetting discussion with possibly more than 8 players? (bb is working on a patch to delete the more options dialog, but we still don't have sufficient space to show all settings simultaenously)

Maybe the best evidence that random maps are intended to adapt to the user settings while atlas maps can't is the feature to launch a game with a random map.

The gamesetup controller can select the "Random" map and change settings freely, then the game selects a random map and that map is supposed to adapt to the given settings.

Overwriting the user choice can work for one or two settings, but the intended direction is the opposite.

Consider a random map enforcing the map size like FeXoR proposed in #1834, or a random map enforcing the victory condition.

Imagine the user can select the "Random" random map and selects regicide, 1v1 on a non-revealed large map, he might start up with a wonder victory game, revealed map on a tiny 6 player map.

So the "Random" item would lie, be ineffective and/or have to be removed.

bb added a comment.Dec 22 2017, 7:50 PM
  • Do you observe a different player behavior with random maps in comparison to scenario and skirmish maps?

can't answer since scenrio's and skirmish maps are played that less often than random maps...

  • Is it a good use case to be able to browse through existing random maps without having the user settings changed?

y, as changing the map, is changing the map, not changing a ton of different random settings implied with that.

  • Does it happen to be a reason that many players only play random maps?

What reason are you referring to? I think the main reason is the possibility of the fact that "every" combination of setting is allowed and that every time you play the same map, it still is different (I never got sick of playing ambush XD)

  • Should we drop all map settings upon map selection for random maps like we do for scenario and skirmish maps?

NONONONONONO

  • Or should we show a chat notification "The map is now revealed" to inform players that a setting was changed behind their back to prevent starting a match with unintended settings after possibly half an hour of gamesetting discussion with possibly more than 8 players?

#chatspam so chat won't be read, imo just don't change settings

FeXoR removed an auditor: leper.May 1 2020, 4:13 PM
FeXoR added a subscriber: FeXoR.

I will remove this concern because it is not specific to this patch.

This does by no means mean I disagree with @leper 's point but rather that this about a design decision: Who should have priority for settings: Player or map (and should there be exceptions) ... and I don't see this being resolved here ;)

This commit no longer requires audit.May 1 2020, 4:13 PM