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

Eternal weather #59707

Merged
merged 13 commits into from
Nov 24, 2022
Merged

Eternal weather #59707

merged 13 commits into from
Nov 24, 2022

Conversation

Night-Pryanik
Copy link

@Night-Pryanik Night-Pryanik commented Jul 28, 2022

Summary

Features "Eternal weather"

Purpose of change

A new external option for players to play with.

Based on @Saicchi's closed #52637, but with several significant changes both on design and implementation.

Regarding @kevingranade's comment from #52637:

I don't like putting this behind an option, it should be possible to enable this via setting a eoc variable in a scenario as in #50685

  1. There are at least 12 types of weather in game. Adding eternal weather through scenarios might be possible, but it would require creating at least 12 new scenarios for every type of weather. I don't think that adding 12 new scenarios, the only distinct feature of each being the eternal weather of specific type, is a good idea.
  2. Even if we add these new scenarios, selecting one of them would block all other scenarios. What if I want to play Helicopter Crash with eternal thunderstorm? It wouldn't possible.

Describe the solution

  • Added new ETERNAL_WEATHER external option.
  • After changing value it requires a restart of the game to take effect.

Describe alternatives you've considered

  • Moving this feature to external options.

Testing

When creating world, set snow storm as eternal weather. Created a character in this world, checked that snow storm is indeed eternal.

Additional context

None.

@Night-Pryanik Night-Pryanik added <Enhancement / Feature> New features, or enhancements on existing [C++] Changes (can be) made in C++. Previously named `Code` Mechanics: Weather Rain, snow, portal storms and non-temperature environment labels Jul 28, 2022
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Jul 28, 2022
@github-actions

This comment was marked as off-topic.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 28, 2022
@pshepjr
Copy link
Contributor

pshepjr commented Jul 28, 2022

I am personally all for any and all additional world creation options, they only add to the replayability and enjoyment of the game!

@Saicchi
Copy link
Contributor

Saicchi commented Jul 28, 2022

Nice.

@PatrikLundell
Copy link
Contributor

  • Wouldn't eternal weather set to "clear" mean bad weather (except portal storms, of course) is disabled, which might be a desirable easy run choice?
  • If acid drizzle/rain actually works (i.e. isn't broken), I don't really see why you would exclude them. I would imagine they'd make the game rather difficult (and boring, if it kills off everything that isn't inside/underground or acid resistant, in which case I'd consider it broken). Some people are masochists...

@Night-Pryanik
Copy link
Author

If player wants to have an easy run, he can set the "Sunny" eternal weather. Setting any type of eternal weather prevents any other weather (portal storms included) from spawning.

I don't know if acid weather is broken or not, if it actually works or not. I don't want to provide dubious, untested choices to players.

@pshepjr
Copy link
Contributor

pshepjr commented Jul 28, 2022

As an aside, eternal acid rain would be a dope challenge run setting. I understand that may be out of the scope of this PR however.

@ghost
Copy link

ghost commented Jul 28, 2022

hell yeah! was looking to spice up my eternal winter challenge, and this would be a good addition to it! thank you!

@Hirmuolio
Copy link
Contributor

Hirmuolio commented Jul 29, 2022

I do not know all the details but sunny weather have some odd features that may need to be worked around for this feature (or you can just remove sunny weather from this).

Sunny weather causes certain things to happen just because it is sunny. Sunlight warmth is applied and extra light is added.
Neither of these check for time of day (the weather code probably does not normally create sunny weather during night. I hope). So sunny weather during night will cause some weirdness.

AFAIK clear weather is "blank" weather that does not have any modifiers. All other weathers either add or remove something. So I do not think it should be removed. If someone wants to play with eternal "clear" then that is fine I think.

src/game.h Outdated Show resolved Hide resolved
@Night-Pryanik
Copy link
Author

Ok, I removed all unneeded json and option copy stuff. Made it so eternal weather updates ingame too. Removed no longer needed debug menu Change weather option as Eternal weather world option is doing essentially the same.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 31, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jul 31, 2022
@kevingranade
Copy link
Member

If 12 new scenarios aren't worth adding why is it meaningful to add them as options? This is symptomatic of the fact that this isn't a curated experience it's just a desire to spam options and have players pick up the pieces. With a trivial amount of effort once the EOC is exposed someone can make the exact scenario they want and that's exactly the kind of thing that we want to encourage for custom play, not having a bunch of switches that encourage people that don't know what they're doing to flip them all.
The situation with combining attributes of scenarios is the same. Eternal Winter is going to absolutely dominate your game so it really doesn't make that much sense to mix it with Helicopter start and if you really want to it's an absolutely trivial amount of JSON editing to get that exact scenario.
It's not a goal of the scenario system to represent every possible start. It's supposed to be a curated list of starts that are going to be interesting in some way and if you want your own we also make it very very easy to make it.

@Night-Pryanik
Copy link
Author

Night-Pryanik commented Aug 25, 2022

why is it meaningful to add them as options?

Because eternal season is an option, not a scenario; eternal time is an option, not a scenario. Following this logic, eternal weather should be an option too. Tying any of this options to scenarios severely limits players choices.

curated experience

What do you mean by that? I don't understand. Care to elaborate?

desire to spam options and have players pick up the pieces.

Is this bad? Players clearly stated that they welcome lots of options they can choose from.

With a trivial amount of effort once the EOC is exposed someone can make the exact scenario they want and that's exactly the kind of thing that we want to encourage for custom play

Not all players have the desire or abilities to create mods. Especially mods with the sole purpose of adding a single scenario. Changing option is billion times easier than creating a mod.

having a bunch of switches that encourage people that don't know what they're doing to flip them all.

You think players are so stupid that they don't understand the description of an option? How about player exactly knows what this particular option do and switches it with full understanding?

Eternal Winter is going to absolutely dominate your game so it really doesn't make that much sense to mix it with Helicopter start

I didn't say a word about eternal season.

What if player wants to have eternal sunny weather? Will eternal sunny weather absolutely dominate his game so it doesn't make sense to mix with any other scenario?


I can hide this feature behind external options. Will you accept it this way?

@pshepjr
Copy link
Contributor

pshepjr commented Aug 25, 2022

I have to chime in and say I also 100% disagree with Kevin's viewpoint on this. With all due respect I am extremely disappointed that this and 'configurable forest' were declined and, frankly, the argument that 'someone can edit this themselves in JSON' is absolutely baffling to me. More options for players should never, ever, be considered a bad thing and expecting them to edit game files is absolutely bizarre.

@anothersimulacrum
Copy link
Member

If we provide an option, it is exceedingly reasonable to assume that it ju-t works, and doesn't break things - e.g. options represent curated things. Options like this thrust the responsibility onto the player to make sure things work, and thus onto us to deal with all the bug reports of various things that don't work if people tweak them. So options like either force us to support a combinatorial explosion of scenarios, or degrade the utility of bug reports as we recieve bug reports to which the answer is "we don't support that" (which is in itself a maintenance burden), and which makes bug reporting less user-friendly.

The expectation is not that users go to edit their JSON for this, but rather that someone puts together a cohesive thing to make it work, and they assume the maintenance burden for that, rather than this project.

@Night-Pryanik
Copy link
Author

We already have a warning in world options menu saying that changing some of these options may lead to unexpected results. We assume that players are not dumb and thus should understand that some combinations of world options, scenarios, professions and so on may lead to weird thing happen. This worked perfect until now, and adding one more option suddenly completely breaks things?
We also didn't suddenly start to thrust the responsibility onto the player to make sure things work with this new option, I don't get it where did you get this statement.

@anothersimulacrum
Copy link
Member

You're right, that is poorly phrased. An option like this either forces the project to maintain and make sure said combinatorial explosion works, or thrust said responsibility onto the player.

Existing options increase maintenance burden, just as this one does. It is the job of project maintainers to decide where to draw the line on what is "worth it".

@Night-Pryanik
Copy link
Author

So, we're not adding this feature in assumption that it might cause bugs? This contradicts our history of adding innumerable unfinished features which indeed caused bugs at the time of merging, but was fixed afterwards.

Unlike eternal season and eternal time of day, this feature has undisputable benefit of turning it off right in the middle of the game, which should immediately revert any possible weird behavior in case it happens.

Even so, I explicitly state that I'm ready to maintain this particular feature. I'll try to fix any bug caused by it. In case I won't be able to fix it, I'm even ready to remove this feature.

@Night-Pryanik
Copy link
Author

I moved this feature from world options to external options.

@Night-Pryanik Night-Pryanik reopened this Aug 27, 2022
@github-actions github-actions bot added the [JSON] Changes (can be) made in JSON label Aug 27, 2022
@github-actions github-actions bot added the <Bugfix> This is a fix for a bug (or closes open issue) label Sep 27, 2022
@I-am-Erk
Copy link
Member

I agree with the argument to keep this in as an option. I think keeping them external is sufficient to meet grounds for stable inclusion, and we can debate the rest later.

@I-am-Erk I-am-Erk merged commit 1f8e0a1 into CleverRaven:master Nov 24, 2022
@Night-Pryanik Night-Pryanik deleted the eternal-weather branch November 25, 2022 06:03
ehughsbaird added a commit to ehughsbaird/Cataclysm-DDA that referenced this pull request Feb 12, 2023
When eternal weather conditions were introduced in CleverRaven#59707, it added this
piece of code to update the weather every turn, presumably to force the
weather to the chosen option on game start, and ensure that anything
that might override it during the game cannot do so.

Unfortunately, updating the weather every turn means that the weather
can oscillate between two types of weather every handful of turns. As
seen in CleverRaven#63072, if these two types of weather have different sight
penalties, this means that the map's level cache will be rebuilt every
couple of turns, significantly slowing down the game.
It also means that weather_manager::nextweather is effectively ignored,
and with it weather_type::duration_min and weather_type::duration_max.

So, instead of forcing a weather update every turn, force a weather
update at the start of the game, and handle eternal weather when
updating weather (giving it priority over weather overrides).
ZhilkinSerg pushed a commit that referenced this pull request Feb 13, 2023
When eternal weather conditions were introduced in #59707, it added this
piece of code to update the weather every turn, presumably to force the
weather to the chosen option on game start, and ensure that anything
that might override it during the game cannot do so.

Unfortunately, updating the weather every turn means that the weather
can oscillate between two types of weather every handful of turns. As
seen in #63072, if these two types of weather have different sight
penalties, this means that the map's level cache will be rebuilt every
couple of turns, significantly slowing down the game.
It also means that weather_manager::nextweather is effectively ignored,
and with it weather_type::duration_min and weather_type::duration_max.

So, instead of forcing a weather update every turn, force a weather
update at the start of the game, and handle eternal weather when
updating weather (giving it priority over weather overrides).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` <Enhancement / Feature> New features, or enhancements on existing [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Mechanics: Weather Rain, snow, portal storms and non-temperature environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants