randomise default air unit elevation#7096
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAirUnit now randomizes its elevation during creation. New configurable fields ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
lua/sim/units/AirUnit.lua (1)
46-59: UsemaxElevationDeltain clamps instead of hardcoded5.
maxElevationDeltais declared but not used, which makes the bounds easy to desync during future edits.Suggested fix
- newElevation = math.min(originalElevation * elevationMultiplier, originalElevation + 5) + newElevation = math.min(originalElevation * elevationMultiplier, originalElevation + maxElevationDelta) else - newElevation = math.max(originalElevation * elevationMultiplier, originalElevation - 5) + newElevation = math.max(originalElevation * elevationMultiplier, originalElevation - maxElevationDelta) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/sim/units/AirUnit.lua` around lines 46 - 59, The clamp uses the literal 5 instead of the declared maxElevationDelta, so update the two math.min/math.max calls in AirUnit.lua to use maxElevationDelta; specifically, where newElevation is assigned in the elevationMultiplier branch, replace "originalElevation + 5" with "originalElevation + maxElevationDelta" and replace "originalElevation - 5" with "originalElevation - maxElevationDelta" so the clamps consistently use the maxElevationDelta variable alongside originalElevation and elevationMultiplier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/sim/units/AirUnit.lua`:
- Around line 50-52: The code dereferences self.Blueprint.Physics.Elevation
without checks; before using blueprintPhysics.Elevation (and before computing
originalElevation and elevationMultiplier) validate that self.Blueprint and
self.Blueprint.Physics exist and that Elevation is a numeric value (use tonumber
or isnumber equivalent) and fall back to a safe default (e.g., 0) if missing,
then ensure maxElevationPercentageDecrease and maxElevationPercentageIncrease
are numeric and clamp their ranges before computing elevationMultiplier; update
references to blueprintPhysics, originalElevation and elevationMultiplier to use
the validated/coerced values.
- Line 52: Replace the unsynced math.random call with the sim-synced RNG
Random() when computing elevationMultiplier (use Random():GetNumber or
project-equivalent to produce a deterministic float in the same range) and
update the expression that reads blueprintPhysics.Elevation to defensively
handle missing blueprint/Physics/Elevation (e.g., read local blueprintPhysics =
self.Blueprint and fallback to 0 or a sensible default if blueprintPhysics or
blueprintPhysics.Physics is nil before using blueprintPhysics.Elevation); adjust
the calculation that sets elevationMultiplier accordingly so it always uses the
synced RNG and a safe elevation value.
---
Nitpick comments:
In `@lua/sim/units/AirUnit.lua`:
- Around line 46-59: The clamp uses the literal 5 instead of the declared
maxElevationDelta, so update the two math.min/math.max calls in AirUnit.lua to
use maxElevationDelta; specifically, where newElevation is assigned in the
elevationMultiplier branch, replace "originalElevation + 5" with
"originalElevation + maxElevationDelta" and replace "originalElevation - 5" with
"originalElevation - maxElevationDelta" so the clamps consistently use the
maxElevationDelta variable alongside originalElevation and elevationMultiplier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
you weren't supposed to include the |
BlackYps
added
area: sim
|
It has been brought to my attention that it would be good to be able to disable this behavior for certain units with a blueprint value. Something like adding the category |
| end | ||
| end, | ||
|
|
||
| --@param self AirUnit |
There was a problem hiding this comment.
This needs to start with 3 --- to be a valid annotation
| RandomiseElevation = function(self) | ||
| local maxElevationDelta = 5 | ||
| local maxElevationPercentageIncrease = 5 | ||
| local maxElevationPercentageDecrease = 5 |
There was a problem hiding this comment.
Why 2 different values for increase and decrease when they are the same?
Its not like a mod could easily adjust this, since its hardcoded in this function
There was a problem hiding this comment.
If I moved these outside of the function, similar to the existing destruction params, would they be able to be changed by mods?
The original version of this had 30 and 0 as the max percentage increase and decrease respectively, this is a holdover
There was a problem hiding this comment.
Yes, if they are saved on the class, mods can easily adjust them without touching the function
There was a problem hiding this comment.
Done. Hopefully this will make it easier to experiment whilst testing as well
| local maxElevationPercentageDecrease = 5 | ||
|
|
||
| local blueprint = self.Blueprint | ||
| local blueprintPhysics = blueprint and blueprint.Physics |
There was a problem hiding this comment.
Check for blueprint is not needed as it's always defined
| local blueprint = self.Blueprint | ||
| local blueprintPhysics = blueprint and blueprint.Physics | ||
| local originalElevation = blueprintPhysics and blueprintPhysics.Elevation | ||
| if type(originalElevation) ~= 'number' then |
There was a problem hiding this comment.
You can just test if an elevation is defined, the type is set by the annotations and if someone decides to put anything else in it, it will most likely complain in the engine before the code gets to Lua
| end, | ||
|
|
||
| --@param self AirUnit | ||
| RandomiseElevation = function(self) |
There was a problem hiding this comment.
It should be Randomize with z. The code is originally and mostly written in american english
| return | ||
| end | ||
|
|
||
| local elevationMultiplier = Random(100-maxElevationPercentageDecrease, 100+maxElevationPercentageIncrease) * 0.01 |
There was a problem hiding this comment.
This will produce 11 different elevation, the original, 5 lower, 5 higher ones. Is that intentional? Or was it suppose to be anything in the range?
There was a problem hiding this comment.
A smooth range is preferable.
Would Random(1-(maxElevationPercentageDecrease/100), 1+(maxElevationPercentageIncrease/100)) create a smooth range?
There was a problem hiding this comment.
Idk, try that and you'll see. But if you give it two integers, it will pick a random integer from that range, not float.
There was a problem hiding this comment.
I've got it working with the entire range now, couldn't tell by eye, so it was checked with debug logs
… have elevation randomised
…random logic to give smooth range
speed2CZ
added
the
area: balance
|
I think this requires a green light from the balance team as well before it can be merged, as this will reduce the effect of AoE of AA weapons. |
|
There's also this console command: |
|
I did some testing and the 5% change is enough that planes clip a lot less while still looking like being at the same general flight level. I like it! |
|
From the discussions with the balance team so far it looks positive, but we should add the "NORANDOMELEVATION" category to transports, to make sure we don't run into weird behaviour with dropping or picking up units. |
|
Would there really be weird behaviour with transports? The elevation at which they fly should not affect this in anyway, since they go into a different state for picking up units, using a different height. And if they fly over hill and take longer to land would be basically the same thing. But Im not against the category being introduced. |
lL1l1
left a comment
lL1l1
left a comment
There was a problem hiding this comment.
Balance-wise this is fine since it has quite a low impact:
- Bombers fly relatively low so they won't move much with 5%, and that small movement transforms into an even less significant bomb drop time change (~0.1s out of 2.26s for ahwassa, the highest flying bomber, and <0.1s changes are undetectable because the game runs in 0.1s ticks).
- ASF at 22 elevation will move up/down 0.9 with 5%, so the lowest flying asf may block the 1.5 radius splash SAM from hitting the highest flying ASF, but that's unlikely and will easily average out over the large numbers built.
Perhaps instead of using a percentage, a proportion of the hitbox could be used as an approximate for the mesh's size. This may result in even smaller variations with even less balance impact while keeping the visual impact.
It would also reasonably restrict the up/down movement for high-flying units.
| -- ELEVATION PARAMS | ||
| MaxElevationDelta = 5, | ||
| MaxElevationPercentageIncrease = 0.05, | ||
| MaxElevationPercentageDecrease = 0.05, | ||
|
|
There was a problem hiding this comment.
If these parameters are never changed in runtime it would be better to assign them in blueprint loading (blueprints-units.lua).
Storing it as an absolute change in the blueprint may also be more convenient for authors who edit their blueprint specifically. You can use the percentage multiplication to calculate that absolute value when it doesn't exist.
Storing it in the blueprint has an added benefit of not requiring a new category to exclude units from the mechanic, since the author can just set the random offset min/max values to 0.
There was a problem hiding this comment.
I agree that per-unit control can be beneficial. I had a look at blueprints-units.lua and I am a bit confused. This seems to mostly alter and process some blueprint values, but very little actual default values. Where are the default values to the various blueprint fields specified that are used when a specific unit doesn't specify that field? I feel like the new variable definitions should go there.
I'm not yet convinced that storing it as an absolute change is really more convenient.
There was a problem hiding this comment.
Similarly, I don't like switching to a proportion of the hitbox. I understand the idea, but I don't see the need to make the variations even smaller, the point of this PR is to create visible variations in the first place, and it could lead to weird effects with bulky units like gunships. Or the Soulripper for that matter. I think just using a flat percentage makes it easier to predict the results.
6 participants
Description of the proposed changes
Randomises aircraft default elevation upwards or downwards 5%. Total change in elevation value cannot be mroe than 5 in either direction.
Testing done on the proposed changes
Additional context
Checklist
Summary by CodeRabbit