fix(weather-editor): type usage and conversions#2200
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR switches several weather-related 8-bit fields from signed to unsigned representations, updates UI sliders and interpolation for wind fields, replaces C-style casts with Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/WeatherEditor/WeatherUtils.cpp (1)
163-182:⚠️ Potential issue | 🔴 CriticalRevert Color3 encoding to signed conversion.
The
Color3::red/green/bluefields in CommonLibSSE-NG arestd::int8_t(signed), not unsigned. UsingFloatToUint8()writes unsigned values (0–255) into signed fields, causing sign-extension corruption. For example,FloatToUint8(0.8f)produces204, which reinterprets in a signed field as–52. RevertFloat3ToColor()and its related overload to useFloatToInt8()andUint8ToFloat()for proper round-tripping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/WeatherUtils.cpp` around lines 163 - 182, Float3ToColor and the Color3 overload of ColorToFloat3 are using unsigned conversions which corrupt signed Color3 fields; change FloatToUint8(...) to FloatToInt8(...) in Float3ToColor and change Uint8ToFloat(...) to Int8ToFloat(...) in the ColorToFloat3 overload that accepts RE::TESWeather::Data::Color3 so the signed std::int8_t Color3.red/green/blue round-trip correctly.src/WeatherEditor/Weather/WeatherWidget.cpp (1)
756-759:⚠️ Potential issue | 🔴 CriticalMissing
uint8_tcast on wind save — same bug thunderLightningFrequency was fixed for.Line 749 casts
data.thunderLightningFrequency(anint8_t) touint8_tbefore storing inweatherProps. Lines 758-759 neglect to do the same fordata.windDirectionanddata.windDirectionRange(alsoint8_tperLerpInt8_tusage insrc/Features/WeatherEditor.cpplines 155-156).When an
int8_tfield storing a 0-255 bit pattern (e.g., 200) is stored directly without casting, it encodes as a negative int (-56). The UI slider (which now usesUINT8_SLIDER, 0-255 range) will clamp this to 0 on next user interaction, permanently corrupting the vanilla heading value.Proposed fix
// Wind weatherProps["Wind Speed"] = data.windSpeed; - weatherProps["Wind Direction"] = data.windDirection; - weatherProps["Wind Direction Range"] = data.windDirectionRange; + weatherProps["Wind Direction"] = static_cast<uint8_t>(data.windDirection); + weatherProps["Wind Direction Range"] = static_cast<uint8_t>(data.windDirectionRange);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 756 - 759, The weatherProps assignments for wind store int8_t fields without casting and must mirror the thunderLightningFrequency fix: when assigning data.windDirection and data.windDirectionRange into weatherProps, explicitly cast them to uint8_t so the stored values represent 0–255 rather than negative signed ints; update the assignments that reference data.windDirection and data.windDirectionRange in WeatherWidget.cpp to cast to (uint8_t) before inserting into weatherProps.
🧹 Nitpick comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
574-584: Minor: unify cast style for int8_t fields on save.Line 574 goes through
static_cast<int8_t>(static_cast<uint8_t>(...))to make the unsigned-bit-pattern → signed-field reinterpret explicit, while lines 583-584 rely on the implicit narrowing from(uint8_t)into theint8_tfield. Consider matching the explicit double-cast on the wind assignments (or dropping the double-cast on the thunder assignment) so the intent is consistent across fields with the same underlying type.♻️ Suggested unification (optional)
- data.thunderLightningFrequency = static_cast<int8_t>(static_cast<uint8_t>(weatherProps["Thunder Lightning Frequency"])); + data.thunderLightningFrequency = static_cast<int8_t>(static_cast<uint8_t>(weatherProps["Thunder Lightning Frequency"])); ... - data.windDirection = (uint8_t)weatherProps["Wind Direction"]; - data.windDirectionRange = (uint8_t)weatherProps["Wind Direction Range"]; + data.windDirection = static_cast<int8_t>(static_cast<uint8_t>(weatherProps["Wind Direction"])); + data.windDirectionRange = static_cast<int8_t>(static_cast<uint8_t>(weatherProps["Wind Direction Range"]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 574 - 584, Unify the cast style so fields with an int8_t storage use the same explicit conversion: replace the C-style casts on data.windSpeed, data.windDirection, and data.windDirectionRange with the explicit two-step static_cast pattern used for data.thunderLightningFrequency (i.e., static_cast<int8_t>(static_cast<uint8_t>(...))) so the unsigned-bit-pattern → signed-field reinterpret is explicit and consistent across these members.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 756-759: The weatherProps assignments for wind store int8_t fields
without casting and must mirror the thunderLightningFrequency fix: when
assigning data.windDirection and data.windDirectionRange into weatherProps,
explicitly cast them to uint8_t so the stored values represent 0–255 rather than
negative signed ints; update the assignments that reference data.windDirection
and data.windDirectionRange in WeatherWidget.cpp to cast to (uint8_t) before
inserting into weatherProps.
In `@src/WeatherEditor/WeatherUtils.cpp`:
- Around line 163-182: Float3ToColor and the Color3 overload of ColorToFloat3
are using unsigned conversions which corrupt signed Color3 fields; change
FloatToUint8(...) to FloatToInt8(...) in Float3ToColor and change
Uint8ToFloat(...) to Int8ToFloat(...) in the ColorToFloat3 overload that accepts
RE::TESWeather::Data::Color3 so the signed std::int8_t Color3.red/green/blue
round-trip correctly.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 574-584: Unify the cast style so fields with an int8_t storage use
the same explicit conversion: replace the C-style casts on data.windSpeed,
data.windDirection, and data.windDirectionRange with the explicit two-step
static_cast pattern used for data.thunderLightningFrequency (i.e.,
static_cast<int8_t>(static_cast<uint8_t>(...))) so the unsigned-bit-pattern →
signed-field reinterpret is explicit and consistent across these members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a3e67d1-69bc-473a-aebb-b7c263c861c2
📒 Files selected for processing (2)
src/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/WeatherUtils.cpp
|
All three review comments are based on the same incorrect premise: that the wind and Color3 fields are signed int8_t |
|
✅ A pre-release build is available for this PR: |
alandtse
changed the title
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 `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 574-584: The load path for windDirection and windDirectionRange
must apply the same unsigned-to-signed cast used for thunderLightningFrequency
to preserve values >=128; update the assignments that read from
weatherProps["Wind Direction"] and weatherProps["Wind Direction Range"] so they
wrap the property in static_cast<uint8_t>(...) and then static_cast<int8_t>(...)
before assigning to data.windDirection and data.windDirectionRange, mirroring
the pattern used for data.thunderLightningFrequency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bbdce35-3564-488b-a7a8-34b5d7a21929
📒 Files selected for processing (2)
src/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
✅ Files skipped from review due to trivial changes (1)
- src/WeatherEditor/Weather/PrecipitationWidget.cpp
(cherry picked from commit 022631b)
2 participants
fixing some issues that were accidentally truncating values.
Summary by CodeRabbit