-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 option to disable weather gloom and rain effects #4302
Conversation
47e69b7
to
e325f59
Compare
@@ -768,7 +768,7 @@ void viewport_paint(rct_viewport* viewport, rct_drawpixelinfo* dpi, int left, in | |||
paint_quadrant_ps(); | |||
|
|||
int weather_colour = WeatherColours[gClimateCurrentWeatherGloom]; | |||
if ((weather_colour != -1) && (!(gCurrentViewportFlags & VIEWPORT_FLAG_INVISIBLE_SPRITES)) && (!(RCT2_GLOBAL(0x9DEA6F, uint8) & 1))){ | |||
if ((weather_colour != -1) && (!(gCurrentViewportFlags & VIEWPORT_FLAG_INVISIBLE_SPRITES)) && (!(RCT2_GLOBAL(0x9DEA6F, uint8) & 1)) && gConfigGeneral.render_weather_gloom){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement would better be spread out over multiple lines to increase legibility.
Hm, I can understand the gloom option, but the weather effects? |
I think it's because of video compression On Thu, Aug 18, 2016, 13:07 Michael Steenbeek notifications@github.com
|
Weather effects tend to have a bad effect on stream quality. |
Ah, that's a good reason. :) (Yay, a Tom Scott link :D ) Still, does this need to be split, though? I'd say one toggle would suffice. |
I have some ideas in mind and have been meaning to respond to the comments but this week I've been working more than usual so i haven't had a chance. I sincerely apologize for such delay. |
2e9e6c1
to
8ffe0fb
Compare
Could you rebase this? |
d4ff2c9
to
7126326
Compare
c528f9e
to
10d66f9
Compare
Okay so I've rebased it and broke the long line (and following long line) for readability sake. it looks a little funky to me so let me know if i can improve the formatting. As far as making the option into one, i've still kept the config file with 2 options, while the in game interface displays one option. this allows power users to adjust if they wish to disable rain but keep gloom (say, someone streaming and they want to show some effect that it's raining).. But I'm not sure on weather this will be okay with everyone. I can see where it would be a bit hogging in the interface, but as far as config options, it's probably not something that's viewed as often I'm also curious on thoughts of making an indented option to allow effects/gloom to be disabled only if in construction mode. |
@@ -768,13 +768,24 @@ void viewport_paint(rct_viewport* viewport, rct_drawpixelinfo* dpi, int left, in | |||
paint_quadrant_ps(); | |||
|
|||
int weather_colour = WeatherColours[gClimateCurrentWeatherGloom]; | |||
if ((weather_colour != -1) && (!(gCurrentViewportFlags & VIEWPORT_FLAG_INVISIBLE_SPRITES)) && (!(RCT2_GLOBAL(0x9DEA6F, uint8) & 1))){ | |||
gfx_fill_rect(dpi2, dpi2->x, dpi2->y, dpi2->width + dpi2->x - 1, dpi2->height + dpi2->y - 1, weather_colour); | |||
if ((weather_colour != -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a newline between the two left parentheses.
Seems to work. 👍
|
I'm not sure they should be. One of the reasons for disabling the weather effects is the effect they have on streaming quality, which, sure, the lightning effect likely also has a detrimental effect on. However, non-streamers may like to disable just the lightning effect, as it may trigger epileptic seizures. I believe this was the main reason it was introduced to begin with. Therefore, I think these options shouldn't be intertwined. |
Uhm, you can still disable only the lighting effects under my plans. What you can't do is turn weather effects off and leave lightning on. |
My point exactly. Why take away rain if you don't have to? |
I think you still don't get what I mean. Under my plans you can either have rain and lightning, rain and no lightning, or no rain and no lightning. |
Right, I understand that. My point is that under what you propose, you will not be able to have rain enabled while lightning is disabled. This, too, is a valid use case. |
Gymnasiast is assuming this checkbox will say "disable rain", but it says "render rain", but i'll switch this around and see if i can't recreate what he's talking about. if not, then i'd propose we go back to a dropdown where it'd allow you to disable lightning, gloom, and raindrops all individually with no restriction. |
@@ -296,15 +296,14 @@ static void climate_update_thunder_sound() | |||
|
|||
static void climate_update_lightning() | |||
{ | |||
if (_lightningTimer == 0) | |||
if (_lightningTimer == 0 || gConfigGeneral.disable_lightning_effect || | |||
(!gConfigGeneral.render_weather_effects && !gConfigGeneral.render_weather_gloom)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this check to:
if (_lightningTimer == 0 || gConfigGeneral.disable_lightning_effect || !gConfigGeneral.render_weather_effects)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gymnasiast .. :/ do i have to? the way it is currently done would benefit the power users who use the ini file, as the in game interface toggles both effects and gloom at the same time (under the disguise as weather effects), where as if someone manually edits the file, i'm sure they'll explicitly want lightning to show if they set it to show. i'd really like if this part stays an exception, unless theres a legit clear reasoning towards the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, alright, let's leave it as it is now, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gymnasiast thanks! I also rebased this as well.
Sorry for the long delay. I have left a line note. In addition to that, could you rebase as well? |
suggestion) Interpreting Gymnasiast part 2
Formatting improvement pt 2
51fcae0
to
9a708cd
Compare
@@ -8,7 +9,7 @@ | |||
- Feature: Add ride console command for diagnostics and changing vehicle type. | |||
- Feature: Allow selecting corners when using the mountain tool. | |||
- Feature: Allow setting ownership of map edges. | |||
- Feature: Allow up to 255 cars per train. | |||
- Feature: Allow up to 255 cars per train and 32 track units per station |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from another PR, maybe you made a mistake while rebasing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was just something i noticed wasn't added into the changelog back when it was implemented. I should probably make that a seperate pull request.
Have no idea if it's just me who gets this (new computer etc so much is different) but there is a huge spacing in between the boarder and text Edit: New computer - Little Time |
If you had read the conversation, you'd know why the checkbox is checked when disabling weather effects. |
i figured that would cause confusion tbh, that's why i was weary about doing it. |
"Disable lightning effect" probably should have been "Render lightning effect". Then it would be common sense that not rendering any weather effects would not render the lightning effect thus not requiring the setting to change value. |
Should I patch it to say render? If I do, would I just change the text and not the setting variable name (and make the in game interface just act as a reverse option?) ? Or should I trash disable_lightning_effect and put in render_lightning_effect? |
I don't think it's really worth the effort. |
I've always hated the gloomy colours when it rains, I mean it's nice when not building anything, but if I'm designing something I want to be able to see it in its entirety but that will be addressed later. I've also noticed that when streaming, if it begins to rain it can add excessive noise to the video stream and distort the image. One could freeze the weather, but that will remove game logic such as plants getting watered, increase in umbrella sales, etc. This allows a user to disable the rendering of the weather gloom and effects while still keeping core gameplay normal.
For now this is not yet finished as I may consider adding that option to only disable gloom when in construction mode. I also noticed that VS merged changes in the solution and project user file, which I need to remove. But for now I'm posting this commit (via phone) because I'm not sure when I will have time to get on my computer again (swing shift jobs suck), and this can get some feedback in the meantime.