Skip to content

Conversation

@chompDev
Copy link
Contributor

No description provided.

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Sep 29, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Encapsulate weather generation state management

Refactor the weather generation logic to encapsulate state management within the
WeatherGenerator. This will remove the tight coupling caused by
RaidWeatherService managing and passing state like presetWeights by reference.

Examples:

Libraries/SPTarkov.Server.Core/Services/RaidWeatherService.cs [41-52]
        WeatherPreset? previousPreset = null;
        var presetWeights = cloner.Clone(weatherGenerator.GetWeatherPresetWeightsBySeason(currentSeason));
        while (nextTimestamp <= futureTimestampToReach)
        {
            // Pass by ref as method will alter weight values
            var newWeatherToAddToCache = weatherGenerator.GenerateWeather(currentSeason, ref presetWeights, nextTimestamp, previousPreset);

            // Add generated weather for time period to cache
            WeatherForecast.Add(newWeatherToAddToCache);


 ... (clipped 2 lines)
Libraries/SPTarkov.Server.Core/Generators/WeatherGenerator.cs [37-42]
    public Weather GenerateWeather(
        Season currentSeason,
        ref Dictionary<WeatherPreset, double> presetWeights,
        long? timestamp = null,
        WeatherPreset? previousPreset = null
    )

Solution Walkthrough:

Before:

// In RaidWeatherService.cs
public void GenerateWeather(Season currentSeason)
{
    WeatherPreset? previousPreset = null;
    var presetWeights = weatherGenerator.GetWeatherPresetWeightsBySeason(currentSeason);
    
    while (forecast_not_full)
    {
        // State (`presetWeights`, `previousPreset`) is managed here and passed to the generator.
        // The `ref` keyword indicates the generator modifies the service's state directly.
        var newWeather = weatherGenerator.GenerateWeather(
            currentSeason, 
            ref presetWeights, 
            nextTimestamp, 
            previousPreset);

        previousPreset = newWeather.SptChosenPreset;
        // ...
    }
}

After:

// In RaidWeatherService.cs
public void GenerateWeather(Season currentSeason)
{
    // The generator now manages its own state for the sequence.
    var weatherSequenceGenerator = weatherGenerator.CreateSequenceGenerator(currentSeason);

    while (forecast_not_full)
    {
        // The service simply requests the next weather object in the sequence.
        var newWeather = weatherSequenceGenerator.GetNext(nextTimestamp);
        // ...
    }
}

// WeatherGenerator would contain the stateful logic
// public class WeatherSequenceGenerator {
//   private Dictionary<WeatherPreset, double> presetWeights;
//   private WeatherPreset? previousPreset;
//   ...
// }
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a significant design flaw where state (presetWeights, previousPreset) is managed across both RaidWeatherService and WeatherGenerator, leading to tight coupling. Encapsulating this state within WeatherGenerator would be a major architectural improvement, simplifying the design and increasing maintainability.

High
Possible issue
Add null safety validation

Replace the null-forgiving operator ! with proper checks to prevent a potential
NullReferenceException if the "default" weather configuration is missing.

Libraries/SPTarkov.Server.Core/Generators/WeatherGenerator.cs [81-83]

-return !WeatherConfig.Weather.WeatherPresetWeight.TryGetValue(currentSeason.ToString(), out var weights)
-    ? WeatherConfig.Weather.WeatherPresetWeight.GetValueOrDefault("default")!
-    : weights;
+if (!WeatherConfig.Weather.WeatherPresetWeight.TryGetValue(currentSeason.ToString(), out var weights))
+{
+    if (!WeatherConfig.Weather.WeatherPresetWeight.TryGetValue("default", out weights))
+    {
+        throw new InvalidOperationException("No weather preset weights found for season and no default configuration available");
+    }
+}
+return weights;
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential NullReferenceException if the "default" weather configuration is missing and proposes adding explicit checks and a meaningful exception, which improves error handling and robustness.

Medium
Add safe parsing validation

Replace double.Parse() with the safer double.TryParse() to prevent potential
runtime exceptions when parsing weather configuration values.

Libraries/SPTarkov.Server.Core/Generators/WeatherGen/AbstractWeatherPresetGeneratorBase.cs [22-25]

 protected double GetWeightedClouds(PresetWeights weather)
 {
-    return double.Parse(weightedRandomHelper.GetWeightedValue(weather.Clouds));
+    var value = weightedRandomHelper.GetWeightedValue(weather.Clouds);
+    if (!double.TryParse(value, out var result))
+    {
+        throw new InvalidOperationException($"Invalid cloud value: {value}");
+    }
+    return result;
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that double.Parse can cause a runtime exception with malformed configuration data and proposes using double.TryParse for safer parsing, which improves the application's robustness.

Low
Learned
best practice
Validate and refresh mutable weights
Suggestion Impact:The commit added a key check before decrementing and clamped the decremented weight to a minimum of zero. It also refined exhaustion handling (checking for zero exactly) though it did not reload from config immediately; instead it clears to trigger reload next time. It partially implements the suggested validations and clamping.

code diff:

         // Only process when we have weights + there was previous preset chosen
-        if (previousPreset.HasValue)
+        if (previousPreset.HasValue && presetWeights.ContainsKey(previousPreset.Value))
         {
             // We know last picked preset, Adjust weights
             // Make it less likely to be picked now
-            presetWeights[previousPreset.Value] -= 1;
-            logger.Info($"{previousPreset.Value} weight reduced by: 1 to: {presetWeights[previousPreset.Value]}");
+            // Clamp to 0
+            presetWeights[previousPreset.Value] = Math.Max(0, presetWeights[previousPreset.Value] - 1);
         }
 
         // Assign value to previousPreset to be picked up next loop
         previousPreset = weightedRandomHelper.GetWeightedValue(presetWeights);
-        logger.Warning($"Chose: {previousPreset}");
 
         // Check if chosen preset has been exhausted and reset if necessary
-        if (presetWeights[previousPreset.Value] <= 0)
+        if (presetWeights[previousPreset.Value] == 0)
         {
-            logger.Info($"{previousPreset.Value} is 0, resetting weights");
             // Flag for fresh presets
             presetWeights.Clear();
         }

Validate presence and non-negative weight before decrement, and clamp to zero to
prevent key errors/negatives; if all weights are zero or dictionary is empty,
refresh from config instead of clearing silently.

Libraries/SPTarkov.Server.Core/Generators/WeatherGenerator.cs [51-69]

-if (previousPreset.HasValue)
+if (previousPreset.HasValue && presetWeights.TryGetValue(previousPreset.Value, out var prevWeight))
 {
-    // We know last picked preset, Adjust weights
-    // Make it less likely to be picked now
-    presetWeights[previousPreset.Value] -= 1;
-    logger.Info($"{previousPreset.Value} weight reduced by: 1 to: {presetWeights[previousPreset.Value]}");
+    var newWeight = Math.Max(0, prevWeight - 1);
+    presetWeights[previousPreset.Value] = newWeight;
+    logger.Info($"{previousPreset.Value} weight adjusted to: {newWeight}");
 }
 
-// Assign value to previousPreset to be picked up next loop
-previousPreset = weightedRandomHelper.GetWeightedValue(presetWeights);
-logger.Warning($"Chose: {previousPreset}");
-
-// Check if chosen preset has been exhausted and reset if necessary
-if (presetWeights[previousPreset.Value] <= 0)
+if (presetWeights.Count == 0 || presetWeights.Values.All(w => w <= 0))
 {
-    logger.Info($"{previousPreset.Value} is 0, resetting weights");
-    // Flag for fresh presets
-    presetWeights.Clear();
+    logger.Info("Preset weights exhausted or empty, reloading from config");
+    presetWeights = cloner.Clone(GetWeatherPresetWeightsBySeason(currentSeason));
 }
 
+var chosen = weightedRandomHelper.GetWeightedValue(presetWeights);
+previousPreset = chosen;
+logger.Info($"Chose: {chosen}");
+
+if (presetWeights.TryGetValue(chosen, out var chosenWeight) && chosenWeight <= 0)
+{
+    logger.Info($"{chosen} weight is 0, reloading weights");
+    presetWeights = cloner.Clone(GetWeatherPresetWeightsBySeason(currentSeason));
+}
+
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Gate high-impact operations behind precise checks and avoid silent state corruption; validate and clamp mutable weighted state before and after mutation.

Low
  • Update

@qodo-merge-for-open-source
Copy link

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: biome-format

Failed stage: Fail PR if Changes Detected [❌]

Failure summary:

The action failed due to JSON formatting issues detected by the BiomeJS formatting step:
- The
workflow formatted files and then checked for uncommitted changes.
- git status --porcelain showed
modifications remained, indicating formatting problems.
- Specifically,
Libraries/SPTarkov.Server.Assets/SPT_Data/configs/weather.json was modified.
- The job emitted an
error: "JSON formatting issues found. Please run BiomeJS locally to format your JSON files and push
the changes to your PR branch." and exited with code 1.

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

221:  shell: /usr/bin/bash -e {0}
222:  ##[endgroup]
223:  Formatted 310 files in 2s. Fixed 1 file.
224:  ##[group]Run rm -f biome.json
225:  �[36;1mrm -f biome.json�[0m
226:  shell: /usr/bin/bash -e {0}
227:  ##[endgroup]
228:  ##[group]Run if [[ -n $(git status --porcelain) ]]; then
229:  �[36;1mif [[ -n $(git status --porcelain) ]]; then�[0m
230:  �[36;1m  echo "changes=true" >> $GITHUB_OUTPUT�[0m
231:  �[36;1melse�[0m
232:  �[36;1m  echo "changes=false" >> $GITHUB_OUTPUT�[0m
233:  �[36;1mfi�[0m
234:  shell: /usr/bin/bash -e {0}
235:  ##[endgroup]
236:  ##[group]Run echo "::error::JSON formatting issues found. Please run BiomeJS locally to format your JSON files and push the changes to your PR branch."
237:  �[36;1mecho "::error::JSON formatting issues found. Please run BiomeJS locally to format your JSON files and push the changes to your PR branch."�[0m
238:  �[36;1mgit status --porcelain�[0m
239:  �[36;1mexit 1�[0m
240:  shell: /usr/bin/bash -e {0}
241:  ##[endgroup]
242:  ##[error]JSON formatting issues found. Please run BiomeJS locally to format your JSON files and push the changes to your PR branch.
243:  M Libraries/SPTarkov.Server.Assets/SPT_Data/configs/weather.json
244:  ##[error]Process completed with exit code 1.
245:  Post job cleanup.

@chompDev chompDev merged commit 836112d into develop Sep 30, 2025
4 of 5 checks passed
@chompDev chompDev deleted the weather-refactor branch September 30, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants