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 world properties (time cycle and weather related features) #2989

Merged
merged 10 commits into from
Jun 30, 2024

Conversation

samr46
Copy link
Contributor

@samr46 samr46 commented May 2, 2023

This adds getWorldProperty, setWorldProperty, resetWorldProperty to modify remaining time cycle and weather related properties. Unlike setTimeCycle from stale PR #276 this implementation is consistent with existing related functions and adds more features.

Imgur album: https://imgur.com/a/jeUxx7L

Property Description
AmbientColor The color of ambient light on map objects (including custom objects)
AmbientObjColor The color of ambient light on dynamically created elements (peds, vehicles)
DirectionalColor The color of direct light on dynamically created elements (peds, vehicles)
Illumination Multiplier for the directional light (DirectionalColor)
SpriteSize Point lights corona size (looks like only traffic lights are affected)
SpriteBrightness Point lights corona brightness (also affects light on ground for light poles)
LightsOnGround Point lights ground reflection brightness (looks like only traffic lights are affected)
PoleShadowStrength Pole shadows alpha (used if volumetric shadows are disabled)
ShadowStrength Shadows alpha (all shadows)
ShadowsOffset Shadows height
Added for convenience to adjust shadows height on demand. Can be used to resolve #2723
BottomCloudsColor Bottom (normal) clouds color
LowCloudsColor Low (skyline) clouds color (seems to be dependant on game hours)
CloudsAlpha Bottom (normal) clouds alpha
WetRoads Wet roads weather effect (mostly noticeable during driving)
Turns on lights on ground during daytime
Foggyness Adds light fog effect for headlights and turns on lights on ground during daytime
Also affects skyline clouds and shadows visibility
Fog Fog weather effect alpha
RainFog Rain fog weather effect alpha (different fog; used in rain weathers) (reset is smooth)
WaterFog Water fog alpha
Rainbow Rainbow alpha
Sandstorm Sandstorm sound volume (reset is smooth)

Some properties like AmbientColor are float by default and adjusted to regular RGB for usability (consistent with timecyc.dat).


Discarded properties:

ExtraSunnyness, CloudCoverage

Both properties work with Foggyness to determine visiblity of skyline clouds. Might actually be necessary if someone wants to use default skyline clouds and alter this behavior of default weathers. But most servers tend to use skydomes with custom clouds these days. And if needed I'd rather we add separate property that could change skyline clouds alpha.

CloudsAlpha2, CloudsAlpha3

Appears to be useless. Changing CloudsAlpha2 doesn't produce any visible results. CloudsAlpha3 slightly changes water fog alpha which is redundant.

Tested and can be added:

TrafficLightsBrightness or TrafficLights

Changes traffic lights ground reflection brightness. By default game uses WetRoads, Foggyness, Rain level and game hour properties to determine if light reflections should be visible. So this could act like main override.


Fixes #385

setWorldProperty("SpriteBrightness", 0)

Fixes #2778

setWorldProperty("ShadowStrength", 500)

Fixes #2785 (kind of)

This function will allow developers to create custom weathers utilizing all time cycle and weather related properties (along with existing functions). So custom blending, 24h time cycles, anything is possible.


Using new parser for this was tricky. But I think it looks good.

It shouldn't be too hard to review. Offsets are used to make memory address validation easier. Test resource with GUI.

Test resource: worldprops.zip

@lopezloo lopezloo added the enhancement New feature or request label May 4, 2023
@Fernando-A-Rocha
Copy link
Contributor

Wow amazing 😄

@FileEX
Copy link
Contributor

FileEX commented Jun 7, 2024

I really don't understand why no one has looked into this for a year. It's a pity that so few people have write access

Copy link
Contributor

@TracerDS TracerDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

Client/game_sa/CWeatherSA.cpp Show resolved Hide resolved
Client/game_sa/CWeatherSA.cpp Show resolved Hide resolved
Client/game_sa/CWeatherSA.cpp Show resolved Hide resolved
Client/game_sa/CWeatherSA.cpp Show resolved Hide resolved
Client/game_sa/CWeatherSA.cpp Show resolved Hide resolved
Client/multiplayer_sa/CMultiplayerSA.cpp Show resolved Hide resolved
Client/multiplayer_sa/CMultiplayerSA.cpp Show resolved Hide resolved
Client/multiplayer_sa/CMultiplayerSA.cpp Show resolved Hide resolved
Client/multiplayer_sa/CMultiplayerSA.cpp Show resolved Hide resolved
Client/multiplayer_sa/CMultiplayerSA.h Outdated Show resolved Hide resolved
@samr46
Copy link
Contributor Author

samr46 commented Jun 7, 2024

@TracerDS
I didn't put memory addresses into definitions (and don't want to) to keep consistency within the file and with current implementation of similar functions. I don't see any reason to use #define for memory addresses in this case because it will make code messy and review harder.

I will make changes to getters and data types later today. Thank you.
Originally I didn't use const and override to make methods as similar as possible to other methods within the file (again for consistency). It actually looks weird right now after we started to use it in newer PRs without proper refactor of old code.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 7, 2024

@TracerDS I didn't put memory addresses into definitions (and don't want to) to keep consistency within the file and with current implementation of similar functions. I don't see any reason to use #define for memory addresses in this case because it will make code messy and review harder.

Magic values are already a hindrance in mta source code. Having them as a definition makes it easier to understand

I will make changes to getters and data types later today. Thank you. Originally I didn't use const and override to make methods as similar as possible to other methods within the file (again for consistency). It actually looks weird right now after we started to use it in newer PRs without proper refactor of old code.

👌

@samr46
Copy link
Contributor Author

samr46 commented Jun 7, 2024

Having them as a definition makes it easier to understand

I actually agree with this and it's useful for reusing address within getter and setter. But it's harder to copy memory address during review / code inspection. And the last one is important since we have PRs without review for years due limited interest / manpower.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 7, 2024

But it's harder to copy memory address during review / code inspection.

How so?

@samr46
Copy link
Contributor Author

samr46 commented Jun 7, 2024

How so?

Double click and Ctrl + C is faster and more intuitive than hovering and clicking Copy or switching directly to #define value. The only benefit is that it can be easier to understand in some cases and less risk of using wrong address in multiple places. In this case all addresses are related to method names so we don't really have any problem with understanding what it will change.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 7, 2024

Double click and Ctrl + C is faster and more intuitive than hovering and clicking Copy or switching directly to #define value. The only benefit is that it can be easier to understand in some cases and less risk of using wrong address in multiple places. In this case all addresses are related to method names so we don't really have any problem with understanding what it will change.

Thats just laziness that doesnt benefit anyone. Memory addresses dont change, definitions do.

@samr46
Copy link
Contributor Author

samr46 commented Jun 7, 2024

Memory addresses dont change, definitions do.

In our case nothing will really change. This is why it's better to keep it consistent imo.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 7, 2024

In our case nothing will really change. This is why it's better to keep it consistent imo.

Thats not consistency. Going with harder to understand codeblocks just because is not a good approach.
Having it defined as a preprocessor variable costs us nothing but benefits exceed the cons.

Magic values are NEVER the way to go

Copy link
Contributor

@FileEX FileEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Debating whether to use a definition for addresses should not be part of this PR. The same applies to discussions about whether to use std library types (recommended for C++) or types from the global namespace (typical for C and available in C++ as well). Such debates should not be part of this PR, as they only lengthen its review. This is not a code refactoring PR. We all know that the MTA code is long spaghetti, but separate PRs are used for refactoring (by the way, they have been waiting for further steps for a long time). Imo code review concerns how the code was written in terms of conventions and consistency with the project, and in this case, consistency was maintained. Mixing it up so that half the code uses definitions and the other half uses direct addresses will only create even more spaghetti.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 7, 2024

Its better to not create problems that would add up in the future. That way there would be less code refactoring PRs.
Better to make changes earlier while there is still chance for merge

@Nico8340
Copy link
Contributor

Nico8340 commented Jun 7, 2024

Its better to not create problems that would add up in the future. That way there would be less code refactoring PRs. Better to make changes earlier while there is still chance for merge

I agree with you on this, the only problem with it is that these are not clarified in the community, they are not stipulated in the coding guidelines.

If these were all clarified and prescribed in a format readable by anyone, there would basically be no problem with it - it would be really useful in the case of a new implementation, preventing further refactoring.

I personally think that to avoid future problems it would be worthwhile to use std namespace types, but it takes a lot of people's opinions to decide that, and this PR is not the place for that.

@FileEX
Copy link
Contributor

FileEX commented Jun 7, 2024

To start with, the coding guidelines haven't been updated for several years. No official documentation is currently up to date regarding the development of the MTA. There is no one who would take care of it and develop it at the appropriate level. Arguing now on every PR will not solve this problem. We should seriously consider developing coding guidelines and establishing some clear community standards.

@samr46
Copy link
Contributor Author

samr46 commented Jun 7, 2024

To start with, the coding guidelines haven't been updated for several years. No official documentation is currently up to date regarding the development of the MTA. There is no one who would take care of it and develop it at the appropriate level. Arguing now on every PR will not solve this problem. We should seriously consider developing coding guidelines and establishing some clear community standards.

It's true. This is why in all my PRs I prefer to make code consistent with existing codebase even when it's not really good in some cases. Without proper refactor many things look out of place.

Copy link
Contributor

@Nico8340 Nico8340 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Copy link
Contributor

@TracerDS TracerDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@samr46
Copy link
Contributor Author

samr46 commented Jun 8, 2024

If someone is interested in merging this for testing in nightly, I will add information to wiki within several hours.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 8, 2024

Resolve conversations, please 😃

@Dutchman101
Copy link
Member

Regarding the coding guidelines, there's been some discussion on dev discord, and contributors took a draft initiative: https://wiki.multitheftauto.com/wiki/New_coding_guidelines

This PR has been reviewed a bunch of times by good people, merging. Thanks to @samr46 for his work to get this highly demanded set of features into MTA.

@Dutchman101 Dutchman101 merged commit a75f1e9 into multitheftauto:master Jun 30, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
9 participants