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.

Sign up for GitHub

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

Jump to bottom

API for light intensity and color at night and day #14091

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

API for light intensity and color at night and day #14091

wants to merge 1 commit into from

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Dec 11, 2023 •
edited

Add compact, short information about your PR for easier understanding:

  • Allows games to change sun color, intensity, and night's darkness.
  • It changed get_sunlight_color function, so no constants are used for sun color calculation, but configurable parameters.
  • This can be useful with mods that control moon phases and weather. It also can be useful in space games.
  • With this, heavy cloudy nights can be darkness than normal nights, etc.

To do

This PR is Ready for Review.

How to test

Run game devtest and run command set_light_intensity from lighting mod.

Screen.Recording.2024-01-02.at.17.47.51_480.mov

@sfan5 sfan5 changed the title Light intensity and color at night and day API for light intensity and color at night and day Dec 12, 2023
@Zughy Zughy added Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap. labels Dec 12, 2023
appgurueu
appgurueu reviewed Dec 28, 2023
View reviewed changes
builtin/game/features.lua Outdated Show resolved Hide resolved
games/devtest/mods/lighting/set_lightIntensity.lua Outdated Show resolved Hide resolved
games/devtest/mods/lighting/set_lightIntensity.lua Outdated Show resolved Hide resolved
games/devtest/mods/lighting/set_lightIntensity.lua Outdated Show resolved Hide resolved
games/devtest/mods/lighting/set_lightIntensity.lua Outdated Show resolved Hide resolved
games/devtest/mods/lighting/set_lighting.lua Outdated Show resolved Hide resolved
games/devtest/mods/lighting/set_lighting.lua Outdated Show resolved Hide resolved
src/lighting.h Outdated Show resolved Hide resolved
Zughy
Zughy reviewed Dec 29, 2023
View reviewed changes
Copy link
Member

@Zughy Zughy left a comment •
edited

Choose a reason for hiding this comment

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

only went through the DOCS. It'd be also nice having some pictures or a small video

doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
@sfence
Copy link
Contributor Author

sfence commented Jan 2, 2024 •
edited

@appgurueu @Zughy Fixes, feedback applied, rebased, and a small video added (to PR itself).

@sfence
Copy link
Contributor Author

sfence commented Feb 3, 2024 •
edited

Rebased, merged to one commit, and cleaned Lua code

@sfence sfence force-pushed the sfence_lightIntensity branch 2 times, most recently from 40144a3 to 4b3ed11 Compare February 23, 2024 18:46
sfan5
sfan5 reviewed Mar 31, 2024
View reviewed changes
src/script/lua_api/l_object.cpp
lua_getfield(L, 3, "color_ratio_coef");
if (lua_istable(L, -1)) {
lighting.lightIntensity.colorRatioCoef_rgb.X = getfloatfield_default(L, -1, "r", lighting.lightIntensity.colorRatioCoef_rgb.X);
lighting.lightIntensity.colorRatioCoef_rgb.Y = getfloatfield_default(L, -1, "g", lighting.lightIntensity.colorRatioCoef_rgb.Y);
Copy link
Member

Choose a reason for hiding this comment

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

these names are bad and way too long

why is it called lightIntensity and not just intensity?
what purpose does the _rgb serve here? I can see that it's a vector, but why actually? We have SColorf.
not to mention that the code style says to name variables_like_this not variablesLikeThis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In mentioning SColorf -> it was discussed here: #14091 (comment).

I just found lightIntensity more understandable than just intensity. No problem to rename.

I use _rgb because it contains three coefficients. One for each color. Also, no problem to rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@appgurueu This is related to our previous discussion.

@sfan5 sfan5 mentioned this pull request Mar 31, 2024
Open
@GefullteTaubenbrust2 GefullteTaubenbrust2 mentioned this pull request Apr 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfan5 sfan5 sfan5 left review comments

@appgurueu appgurueu appgurueu left review comments

@Zughy Zughy Zughy left review comments

Assignees
No one assigned
Labels
@ Client rendering Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap. @ Script API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sfence @sfan5 @appgurueu @Zughy @wsor4035