Tweak the formula used to calculate the level of detail (LOD) for props.
Description of the proposed changes
As per the title. Screenshots can be found below.
Checklist
- [x] Changes are annotated, including comments where useful
- [x] Changes are documented in a changelog snippet according to the guidelines.
- [x] Request 2-3 reviewers from the list of reviewers and their areas of knowledge.
Summary by CodeRabbit
- Bug Fixes
- Improved prop level-of-detail (LOD): LOD now scales with prop size, adds explicit cutoffs for certain tree types, enforces a maximum for others, redistributes per-LOD cutoffs proportionally, and refines rounding so cutoffs snap to the nearest 10 — reducing pop-ins and making broken tree groups easier to spot.
- Tweaks
- Adjusted physical sizes for some deposit props so their visibility and interaction better match intended scale.
- Changelog
- Added entry describing the LOD tweak and its visible effects.
✏️ Tip: You can customize this high-level summary in your review settings.
I'm not sure if this is going in a direction that is helpful to address the root problem. In theory the changing of LOD levels should ideally be barely noticeable to the user. When the lowest level looks better than the level before that, then we should investigate there.
I'm not sure if this is going in a direction that is helpful to address the root problem.
@BlackYps This PR is designed to only give us an additional knob to play with. Right now, the blueprint values are skipped entirely, but I think it would be beneficial to at least have the option for blueprint values to take precedence, if desired.
In theory the changing of LOD levels should ideally be barely noticeable to the user. When the lowest level looks better than the level before that, then we should investigate there.
After some additional testing and further contemplation, I think the way the second LOD looks is fine, and the issue is that it’s currently being used at too-high zoom levels. I addressed this in my other PR.
This is the maximum zoom level at which the second LOD level is currently visible:
As you can see, tree groups are more difficult to identify, and the overall look feels a bit out of place.
Displaying the most detailed LOD level for longer
You made the suggestion, that the second, less detailed LOD level could be removed entirely. I found the idea intriguing, so I tested it.
With the second LOD level retained:
With the second LOD level removed:
Visually, I prefer the first screenshot (with the second LOD level retained). Removing it, however, eliminates the transition between the LOD levels, which looks smoother and more modern - albeit with a potential performance cost that still needs to be investigated.
In summary, I think a good argument can be made for both keeping and removing the second LOD. I would prefer to keep it for now and make the LOD behavior more similar to how it was pre-patch, while retaining the increased maximum render distance of trees (which is what I am doing here and in #6907).
If the goal is to prefer showing higher LODs sooner then you can also adjust the formula found here:
https://github.com/FAForever/fa/blob/3bad1c40cf8465bf71445d85180d36bf0317d64e/lua/system/blueprints-props.lua#L115-L137
If you change local factor = (k / n) * (k / n) to local factor = (k / n) * (k / n) * (k / n) it will preference the higher LODs while the maximum render distance remains unchanged. Additionally, you can change add 10 to the sanitized result. As an example:
INFO: /env/redrocks/props/trees/groups/redtree01_group1_prop.bp LOD: 1 4.3744492530823 -> 1.4581497907639
INFO: /env/redrocks/props/trees/groups/redtree01_group1_prop.bp LOD: 2 69.991188049316 -> 46.660793304443
INFO: /env/redrocks/props/trees/groups/redtree01_group1_prop.bp LOD: 3 354.3303527832 -> 354.3303527832
INFO: /env/evergreen/props/trees/groups/pine06_big_groupb_prop.bp LOD: 1 6.8673510551453 -> 2.2891170978546
INFO: /env/evergreen/props/trees/groups/pine06_big_groupb_prop.bp LOD: 2 109.87761688232 -> 73.251747131348
INFO: /env/evergreen/props/trees/groups/pine06_big_groupb_prop.bp LOD: 3 556.25537109375 -> 556.25537109375
Looking at these results it may also be interesting to add a flat value of 10 to each level. Because a LODCutoff of 4.3 won't show up any time soon, let alone 1.45 😃 !
@Garanas Good catch, local factor = (k / n) * (k / n) * (k / n) was the first thing I tried when making the PR, but thanks for steering me back toward the correct solution anyway. I tried local factor = (k / n) * (k / n) * (k / n) * (k / n) now, and it reverts the starting zoom level for the least detailed LOD back to how it was pre-patch. Since this is a more aggressive formula, I also went with adding a flat value of 30 instead of 10.
It looks promising on Seton's, but I haven't tested very many other maps yet. I think red maps still need some tuning, the most detailed LOD of a lot of the trees there is still not visible for very long.
@Rhaelya Can you give these changes a try to see if this does what you are looking for? I did some preliminary testing on Seton's, and I think this is the outcome we're after. You won't need the changes from #6907 to test the new LOD levels now.
I did some more testing on a variety of maps, and found no glaring issues. If the skip the automatic computation of LOD part of the PR isn't liked, I'll revert it. I think it's a fine idea but not essential.
@BlackYps Since you are more experienced than I am, can you take another look at this?
Sorry, this conversation stalled a bit. I'd say we should change much of how this whole function works. Instead of doing indirect changes to the mathematical formula, we should define explicit ranges, for example 50, 100, 200 and then we scale these according to the prop size. This way you have more control over when which LOD level shows and the code is easier to read as well. If we define separate values for tree groups and single trees we can get back the behaviour that you can spot broken tree groups by noticing the LOD change.
In the end this should all be baked in the blueprint of the props. I remember there is a tool for it, but I don't remember any details. I think @Garanas should know.
I'd say we should change much of how this whole function works. Instead of doing indirect changes to the mathematical formula, we should define explicit ranges, for example 50, 100, 200 and then we scale these according to the prop size. This way you have more control over when which LOD level shows and the code is easier to read as well.
I like this idea!
There's a workflow that runs the blueprints.lua file and bakes properties accordingly. See also:
- https://github.com/FAForever/fa/blob/develop/.github/workflows/bake-blueprints.yaml
It creates a new pull request with the new baked in results. Therefore we'd first need to merge this PR to adjust the formula. Then run the workflow to bake it into the blueprints, which opens as a new pull request. For more information:
- https://github.com/FAForever/fa/pull/6752
@Basilisk3 do you intend to finish this? Otherwise I would pick it up
Walkthrough
Compute a size-weighted metric from sx, sy, sz to set maxLod (180 * weightedLodSize); for Tree/TreeGroup with exactly 3 LODs use cutoffs [40, 165, 640] and return, otherwise cap maxLod at 640. Per-LOD cutoffs are (n / levels) * maxLod, rounded to nearest 10. Also update sizes for two prop blueprints and add a changelog entry.
Changes
| Cohort / File(s) | Summary |
|---|---|
LOD Calculation & Changelog changelog/snippets/feature.6906.md, lua/system/blueprints-props.lua |
Add changelog entry. Replace previous LOD formula with a size-weighted approach: compute weightedLodSize from sx, sy, sz, set maxLod = 180 * weightedLodSize. For Tree/TreeGroup with exactly 3 LODs set explicit LODCutoff [40, 165, 640] and return; otherwise cap maxLod at 640. Compute per-LOD LODCutoff = (n / levels) * maxLod and round to nearest 10 using floor((LODCutoff + 5) / 10) * 10. No public API changes. |
Prop Blueprint Size Updates env/Common/Props/hydrocarbonDeposit01_prop.bp, env/Common/Props/massDeposit01_prop.bp |
Adjust prop size fields: hydrocarbonDeposit01_prop.bp SizeX, SizeY, SizeZ changed from (0.1, 0.1, 0.1) to (1.0, 0.2, 1.0); massDeposit01_prop.bp SizeX, SizeZ increased from 0.1 to 0.9. Collision/other fields unchanged. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Review
weightedLodSizecomputation and units (sx/sy/sz use and weighting). - Verify interaction between the 180 multiplier and the 640 cap for edge cases.
- Confirm Tree/TreeGroup 3-LOD hardcoded cutoffs and early-return are intended.
- Check rounding behavior near boundary values and for very small props.
- Validate that the prop size changes align with intended visual/collision outcomes.
Poem
🐇 I measured x, y, z with a curious paw,
Tuned cutoffs by size and skipped a flaw.
Three trees sing at forty, one-sixty-five, six-forty bright,
Small bumps grown broader, now seen in light.
A rabbit hops — LODs look just right 🌿
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | ⚠️ Warning | The description is incomplete. It references screenshots without embedding them, lacks detailed explanation of the changes, and provides minimal testing details despite the changelog indicating preliminary testing on multiple maps. | Expand description with technical details of the formula changes, embed or link screenshots, describe testing scope (maps tested, prop types), and clarify the size adjustment rationale for deposit props. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: a tweak to the LOD calculation formula for props, which is the primary focus of the changeset. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 63c8e790a06bdbf35fdf525ed0f1d387562d9dcc and e888cf7f14708f4866512391000cd595bfc06e24.
📒 Files selected for processing (2)
env/Common/Props/hydrocarbonDeposit01_prop.bp(1 hunks)env/Common/Props/massDeposit01_prop.bp(1 hunks)
🔇 Additional comments (2)
env/Common/Props/massDeposit01_prop.bp (1)
28-30: LGTM - Size adjustments look appropriate for deposit props.The size increases (0.1 → 0.9 for X and Z dimensions) address the unrealistically small bounding boxes mentioned in the PR discussion. Since this prop is invulnerable and untargetable, the collision detection changes carry minimal risk.
Please verify in-game that the new bounding box dimensions appear correct and don't cause any visual or interaction issues with the deposit model.
env/Common/Props/hydrocarbonDeposit01_prop.bp (1)
31-33: LGTM - Hydrocarbon deposit size adjustments consistent with mass deposit changes.The size increases (SizeX and SizeZ to 1.0, SizeY to 0.2) mirror the mass deposit adjustments and fix the same unrealistic tiny bounding box issue. The invulnerability ensures collision changes won't introduce gameplay bugs.
Please verify in-game that the new dimensions align properly with the hydrocarbon deposit model and don't cause visual artifacts or interaction problems.
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 help to get the list of available commands and usage tips.
Yes, I intend to finish it. The ability for props to skip the automatic computation of LOD still seems useful to me, but if it's controversial I can revert it to get the more important change (the prop LOD changes) ready. When I tested the LOD changes a while ago, I found no glaring issues, but it would probably be beneficial if someone with more experience in this part of the game took another look.
These are the LOD changes I mean.
So basically this only needs some more feedback and a decision on whether we want to keep the new category that allows us to potentially skip the automatic computation of LODs for select props.
I posted what I think is the way forward here: https://github.com/FAForever/fa/pull/6906#issuecomment-3418753558 I don't think we need the category. Ideally we come up with a formula that gives us good results, so we eliminate the desire to finetune individual props.
While you are at it, I think this formula should be adjusted: local weighted = 0.40 * sx + 0.2 * sy + 0.4 * sz. It seems very unusual to me to just add the dimensions. I am okay with giving less emphasize to the height, but it would make more sense to me to calculate the diagonal length for example.
I removed the new category. As for the weighted formula, I tried a couple of options, but I think the current one works well enough - at least on the maps I tested.
I read the whole conversation again, but it's a bit unclear what exactly the goal is. I figured you want to set the cutoff of the first lod level to where it was before we increased the weighted value for trees from 2.6 to 10? And what maps did you test on? I will soon push a commit to simplify the formulas, so it would be good to know what you are aiming for.
My reasoning can be found here: https://github.com/FAForever/fa/pull/6907. The goal is to revert tree group visibility to what it was pre‑patch, while keeping the higher max LOD level the patch introduced. As you can see in the first screenshot, tree group visibility is currently worse than it was pre‑patch, which this PR aims to fix.
I mostly investigated the evergreen props found on maps such as Seton's and Selkie, since these maps and their props are the most popular. Red maps behaved fine as well, though I didn’t check those as thoroughly.
I changed the function so it now allows setting explicit values for tree groups and single trees. The tree groups only use lod1 and lod3. This way you can spot broken tree groups because the individual trees render with lod2 on medium zoom ranges. Tested on selkie and the fa and faf props test map. I believe this should solve the problem of not being able to spot broken tree groups.
Unrelated to the tree changes: big props are now visible for longer.
This way you can spot broken tree groups because the individual trees render with lod2 on medium zoom ranges.
While this is a good idea in principle, I don't think it's necessary, as you can spot broken tree groups already.
Below is the current state of the PR. As you can see, the LOD levels are now inconsistent depending on where you center the camera (see the top left). Additionally, I think keeping the second LOD level in place looks nicer overall, as shown here. This is because the colors of the first and last LOD levels look quite different, and the second LOD level helps ease the transition.
@BlackYps If you want, you can ping me on Discord and I will try to reply as soon as I can. These AI comments are making this pretty convoluted.
I changed the function so it now allows setting explicit values for tree groups and single trees. The tree groups only use lod1 and lod3. This way you can spot broken tree groups because the individual trees render with lod2 on medium zoom ranges. Tested on selkie and the fa and faf props test map. I believe this should solve the problem of not being able to spot broken tree groups.
Unrelated to the tree changes: big props are now visible for longer.
I don't think it was mentioned in this thread but the breaking of tree groups updates through the fog of war. Have you checked whether this makes it (too) obvious to spot raids through the fog of war at certain camera angles?
As an example, take this forum post:
- https://forum.faforever.com/topic/3852/scouts-and-labs-should-not-break-tree-groups/1
The problem will always be there of course. But before the aim was to minimize it, which was accomplished by having individual trees and tree groups always share the same lod settings.
I investigated some more and I am now convinced that my idea to show a different lod for groups and single trees is based on a misunderstanding when I was discussing the issue with Rhaelya. We were convinced that the visible difference between both was because of different lods being visible. But I think we were wrong. It's hard to get a good understanding of what lod level exactly you are seeing when zooming around in a map. I did some tests and found that there are minute differences in color in the lod3 of groups and single trees. So there is no reason to set different lod values for both. I will remove that piece of code again.
This is not related to any changes in the PR, but another thing I noticed while playing, is that tree groups sometimes have difficulties rendering when terrain is uneven.
Visible when comparing the bottom center tree groups. This only appears at certain zoom levels.
That's probably because of the terrain tesselation and unavoidable
Looks good to me now
The mass and hydrocarbon deposit props now only appear at very low zoom levels. Apart from that, I think the PR is ready.
Has this not always been the case?
Yes, but not to this extent.
Found the issue. massDeposit01_prop.bp defines:
SizeX = 0.1,
SizeY = 0.1,
SizeZ = 0.1,
Same for the hydro markers. Do we expect any side effects when increasing these values?
These values are used for collision detection. For splash damage, for example. But the deposit props are immune to damage. I don't think there are significant side effects.