Add a setter and getter for the intensity value of ColorPicker.#1217
Open
Tekkitslime wants to merge 4 commits intoRedot-Engine:masterfrom
Open
Add a setter and getter for the intensity value of ColorPicker.#1217Tekkitslime wants to merge 4 commits intoRedot-Engine:masterfrom
Tekkitslime wants to merge 4 commits intoRedot-Engine:masterfrom
Conversation
WalkthroughAdds a new public float property Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scene/gui/color_picker.cpp`:
- Line 2112: The property registration uses Variant::INT which truncates
fractional values; change the ADD_PROPERTY call for "intensity" to use
Variant::FLOAT so it matches the set_intensity/get_intensity methods and the UI
slider/documentation; update the PropertyInfo in the ADD_PROPERTY(
PropertyInfo(..., "intensity"), "set_intensity", "get_intensity" ) invocation to
Variant::FLOAT to preserve decimals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b027ec7f-8c56-4ef3-b84c-ebea5235bc2c
📒 Files selected for processing (3)
doc/classes/ColorPicker.xmlscene/gui/color_picker.cppscene/gui/color_picker.h
Contributor
|
Hey Tekk, It looks like it is failing the Unit Tests, I think you will need to update those to account for the new method. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scene/gui/color_picker.cpp (1)
2114-2114:⚠️ Potential issue | 🟠 MajorRegister
intensityas a float property.
set_intensity()andget_intensity()usefloat, so exposing the property asVariant::INTwill truncate fractional values through property/script access.Suggested fix
- ADD_PROPERTY(PropertyInfo(Variant::INT, "intensity"), "set_intensity", "get_intensity"); + ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "intensity"), "set_intensity", "get_intensity");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scene/gui/color_picker.cpp` at line 2114, The property is registered as Variant::INT which truncates fractional values; update the ADD_PROPERTY registration to use a floating Variant type (Variant::REAL) so the exposed "intensity" property matches set_intensity/get_intensity (both use float) — locate the ADD_PROPERTY(PropertyInfo(..., "intensity"), "set_intensity", "get_intensity") call and change the PropertyInfo's Variant::INT to Variant::REAL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scene/gui/color_picker.cpp`:
- Around line 2096-2097: The new bindings add script-visible methods
ColorPicker::set_intensity and ColorPicker::get_intensity, so update the
API/unit test baseline that enumerates ColorPicker methods to include these two
entries; locate the test that asserts the class method list or the bindings
snapshot for ColorPicker and add expectations for "set_intensity" and
"get_intensity" (or regenerate the baseline snapshot) so the unit tests reflect
the new API surface.
---
Duplicate comments:
In `@scene/gui/color_picker.cpp`:
- Line 2114: The property is registered as Variant::INT which truncates
fractional values; update the ADD_PROPERTY registration to use a floating
Variant type (Variant::REAL) so the exposed "intensity" property matches
set_intensity/get_intensity (both use float) — locate the
ADD_PROPERTY(PropertyInfo(..., "intensity"), "set_intensity", "get_intensity")
call and change the PropertyInfo's Variant::INT to Variant::REAL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a17016d6-6c01-4ce2-a97d-647967e2deb5
📒 Files selected for processing (1)
scene/gui/color_picker.cpp
Comment on lines
+2096
to
+2097
| ClassDB::bind_method(D_METHOD("set_intensity", "intensity"), &ColorPicker::set_intensity); | ||
| ClassDB::bind_method(D_METHOD("get_intensity"), &ColorPicker::get_intensity); |
There was a problem hiding this comment.
Please update the API test expectations for the new bindings.
This adds new script-visible methods, and the PR context already notes unit tests are failing. The corresponding API/unit test baseline needs to be updated alongside these bindings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scene/gui/color_picker.cpp` around lines 2096 - 2097, The new bindings add
script-visible methods ColorPicker::set_intensity and
ColorPicker::get_intensity, so update the API/unit test baseline that enumerates
ColorPicker methods to include these two entries; locate the test that asserts
the class method list or the bindings snapshot for ColorPicker and add
expectations for "set_intensity" and "get_intensity" (or regenerate the baseline
snapshot) so the unit tests reflect the new API surface.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scene/gui/color_picker.cpp`:
- Around line 376-380: set_intensity currently assigns the backing field
directly (intensity) which bypasses the UI control logic and range/coercion done
by the slider/spinbox, so update the setter to route through the existing
control path: either call the shared helper that applies intensity (extract
_normalized_apply_intensity_to_color logic into a new helper and call it from
both set_intensity and _slider_value_changed) or set the slider/spinbox value
programmatically so _slider_value_changed runs; ensure you still call
_update_color afterwards and reference the functions set_intensity,
_normalized_apply_intensity_to_color, _slider_value_changed, and _update_color
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aac2b3ab-ac18-4509-8bc5-e916ef9bc97e
📒 Files selected for processing (1)
scene/gui/color_picker.cpp
Arctis-Fireblight
previously requested changes
Mar 8, 2026
…s min and max bounds, reflect that in the docs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
doc/classes/ColorPicker.xml (1)
85-87: Document whatintensityactually means.This entry tells users that the property exists, but not how the value maps to the picked color. Since
intensityis now script-facing API, it should mirror the semantic explanation already given onedit_intensity, and ideally phrase the clamp note so it does not become a second source of truth if the slider bounds ever change.📝 Suggested doc update
<member name="intensity" type="float" setter="set_intensity" getter="get_intensity" default="0.0"> - The currently selected intensity. - Note: [code]set_intensity[/code] clamps the value in the range [code][-10.0, 10.0][/code] + The currently selected intensity. This is the same value shown by the intensity slider; it multiplies the color by [code]2 ** intensity[/code] in linear RGB space, and then converts it back to sRGB. + [b]Note:[/b] [code]set_intensity()[/code] clamps the value to the intensity slider range (currently [code][-10.0, 10.0][/code]). </member>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/classes/ColorPicker.xml` around lines 85 - 87, The documentation for the script-facing property intensity is missing semantic meaning and duplicates clamp bounds; update the <member name="intensity"> docs to explain what intensity controls (e.g., that it shifts the picked color toward white/darker/ambient or adjusts brightness/saturation as described by edit_intensity), mirror the same explanatory text used in edit_intensity, and replace the hard-coded clamp note with a statement like “value is clamped to the same range enforced by set_intensity/GUI slider” referencing set_intensity and the slider rather than repeating numeric bounds so the documentation remains correct if the slider limits change; ensure get_intensity and set_intensity are mentioned to indicate how scripts read/write the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@doc/classes/ColorPicker.xml`:
- Around line 85-87: The documentation for the script-facing property intensity
is missing semantic meaning and duplicates clamp bounds; update the <member
name="intensity"> docs to explain what intensity controls (e.g., that it shifts
the picked color toward white/darker/ambient or adjusts brightness/saturation as
described by edit_intensity), mirror the same explanatory text used in
edit_intensity, and replace the hard-coded clamp note with a statement like
“value is clamped to the same range enforced by set_intensity/GUI slider”
referencing set_intensity and the slider rather than repeating numeric bounds so
the documentation remains correct if the slider limits change; ensure
get_intensity and set_intensity are mentioned to indicate how scripts read/write
the value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46907370-c805-4128-aa74-2c26904efa6e
📒 Files selected for processing (2)
doc/classes/ColorPicker.xmlscene/gui/color_picker.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- scene/gui/color_picker.cpp
Arctis-Fireblight
dismissed
their stale review
Requested changes were made, review is outdated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit adds a getter and setter for ColorPicker's intensity value. The setter works by setting the intensity through the slider, so that everything updates as expected. This closes Redot-Engine/redot-proposals#117
Summary by CodeRabbit