Tweak the formula used to calculate the level of detail (LOD) for props. #6906
Conversation
Basilisk3
added
area: graphics
|
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. |
@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.
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 longerYou 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: fa/lua/system/blueprints-props.lua Lines 115 to 137 in 3bad1c4 If you change 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, 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 @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. 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 like this idea! There's a workflow that runs the blueprints.lua file and bakes properties accordingly. See also: 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: |
|
@Basilisk3 do you intend to finish this? Otherwise I would pick it up |
WalkthroughCompute a size-weighted metric from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (2)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelog/snippets/feature.6906.md(1 hunks)engine/Core/Categories.lua(1 hunks)lua/system/blueprints-props.lua(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Documentation - Generate changelog
changelog/snippets/feature.6906.md
[error] 1-1: Changelog verification failed. Invalid format: feature.6906.md. Command 'changelog-verify.sh' exited with code 1.
🔇 Additional comments (1)
lua/system/blueprints-props.lua (1)
124-126: LGTM: Blueprint LOD precedence logic is correct.The conditional check properly skips automatic LOD computation when the
USEBLUEPRINTLODcategory is present, allowing blueprint-defined values to take precedence. The logic is sound and aligns with the PR objectives.
|
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: #6906 (comment) |
|
While you are at it, I think this formula should be adjusted: |
|
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. |
Basilisk3
changed the title
|
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? |
|
My reasoning can be found here: #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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/system/blueprints-props.lua (1)
63-74:isReclaimableuses the wrong category constantLine 64 duplicates the
OBSTRUCTSBUILDINGcheck instead of using a reclaim-related category, so the warning branch can never trigger as intended.- local isReclaimable = TableFind(prop.Categories, 'OBSTRUCTSBUILDING') + local isReclaimable = TableFind(prop.Categories, 'RECLAIMABLE')This restores the intended distinction between “blocks building” and “is reclaimable” when emitting warnings.
♻️ Duplicate comments (1)
lua/system/blueprints-props.lua (1)
158-168: Global LOD formula change still requires re-running the bake-blueprints workflow
ProcessLODis used both at runtime and inBakePropBlueprints, so this new formula will only be persisted to blueprints after the.github/workflows/bake-blueprints.yamlworkflow is run again.As with the previous iteration of this PR, please ensure the bake workflow is triggered after merge (or as a follow-up PR) so all prop blueprints get updated LODCutoff values.
🧹 Nitpick comments (2)
lua/system/blueprints-props.lua (2)
112-119: Confirm the180 * weightedLodSizescaling and consider an upper clampThe new metric
sqrt(sx^2 + 0.5 * sy^2 + sz^2)with a global factor of 180 can yield very largemaxLodvalues for big props, since only trees are clamped to 640 later. This may push far-LOD visibility to extreme distances and impact performance.Consider either:
- adding a global upper bound for
maxLod(similar to trees), or- documenting why unbounded
maxLodis acceptable for non-tree props.
134-142: Factor-based distribution is now strictly linear; ensure this matches the desired bias toward higher-detail LODsThe new per-level formula:
local factor = (n / levels) local LODCutoff = factor * maxLod data.LODCutoff = MathFloor((LODCutoff + 5) / 10) * 10produces evenly spaced cutoffs (linear in
n) with rounding to the nearest 10. Earlier discussions in the PR referenced higher-order factors to keep high-detail LODs visible longer.If the design goal is still to favor higher-detail LODs (slower drop-off of detail), consider whether a non-linear factor (e.g.,
(n / levels)^kwithk > 1or a small offset) would align better with the intended visual behavior, or add a short comment documenting why linear spacing is now preferred.
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. |
This reverts commit 884e37c.
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: 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. |
lua/system/blueprints-props.lua
Outdated
| prop.Display.Mesh.LODs[2].LODCutoff = 200 | ||
| prop.Display.Mesh.LODs[3].LODCutoff = 640 | ||
| return | ||
| elseif (prop.ScriptClass == 'TreeGroup' and TableGetn(prop.Display.Mesh.LODs) == 3) then |
There was a problem hiding this comment.
The use of elseif after a return statement is confusing. It gives the impression that the function can continue. The return will already make you exit the function.
It's better to just write:
if (...) then
(...)
return
end
if (...) then
(...)
return
end
-- default case when no special rules apply
(...)
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lua/system/blueprints-props.lua (1)
112-119: LOD scaling and rounding logic look sound; remember to re‑bake blueprints.Using a diagonal-like size metric to derive
maxLodand spacing cutoffs linearly byn / levelsgives monotonic, size-aware LOD distances, and rounding withMathFloor((LODCutoff + 5) / 10) * 10cleanly snaps to the nearest 10 while preserving ordering. SinceProcessLODis also called fromBakePropBlueprints, merging this will change bakedLODCutoffvalues across all props, so ensure the bake-blueprints workflow is triggered once the final formula is settled.Also applies to: 132-139
🧹 Nitpick comments (1)
lua/system/blueprints-props.lua (1)
120-130: Clarify Tree/TreeGroup special-case control flow and reuselevels.Functionally this branch is fine (3‑LOD trees get the explicit
[40, 180, 640]distances, others are capped viamaxLod = 640), but the nestedifplus callingTableGetnagain later makes the flow a bit harder to follow. Consider hoistinglocal levels = TableGetn(prop.Display.Mesh.LODs)once and structuring it as sequential guards (if isTree and levels == 3 then ... return end/if isTree then maxLod = 640 end) so the early-return and fallback are more obvious.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lua/system/blueprints-props.lua (1)
112-118: Document the weighted size formula and the 200 multiplier.The diagonal-based
weightedLodSizeformula aligns with the recommendation to move toward a length-based measure, and reducing the Y-axis weight (0.5 coefficient) is reasonable given typical camera angles. However, the rationale for the specific formula and the200multiplier should be documented inline to help future maintainers understand the tuning choices.Consider adding a comment:
local sx = prop.SizeX or 1 local sy = prop.SizeY or 1 local sz = prop.SizeZ or 1 - -- give more emphasis to the x / z value as that is easier to see in the average camera angle + -- Compute a weighted diagonal size; Y (height) has reduced weight (0.5) + -- because it's less prominent from typical camera angles local weightedLodSize = MathSqrt(sx * sx + 0.5 * sy * sy + sz * sz) + -- Scale to maximum LOD distance; 200 multiplier tuned for visual quality vs. performance local maxLod = 200 * weightedLodSize
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lua/system/blueprints-props.lua(1 hunks)
🔇 Additional comments (1)
lua/system/blueprints-props.lua (1)
131-138: Verify that the linear LOD factor matches the intended formula.The current implementation uses a simple linear factor
(n / levels)with no polynomial exponent or flat offset:local factor = (n / levels) local LODCutoff = factor * maxLodHowever, the PR discussion and past review comments reference testing with higher-power exponents (e.g.,
(k/n)^3or(k/n)^4) and adding flat offsets (e.g.,+30) to bias toward higher-detail LODs and prevent small cutoff values. The current linear formula produces a different progression than those approaches.Positive: The rounding change from "always round up" to "round to nearest 10" (line 138) is an improvement that produces more accurate cutoff values.
Please confirm whether:
- The linear formula is the final intended design after testing various exponents, or
- The polynomial exponent and offset were accidentally omitted during recent changes.
If the linear formula is intentional, consider adding a comment explaining that decision (e.g., "Linear progression chosen after testing for smoother LOD transitions").
Basilisk3
left a comment
Basilisk3
left a comment
There was a problem hiding this comment.
Apart from that lgtm.
|
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/system/blueprints-props.lua (1)
63-74: Bug:isReclaimablecheck is broken, warning can never triggerThis isn’t part of the new changes, but it’s a correctness issue:
local isObstructing = TableFind(prop.Categories, 'OBSTRUCTSBUILDING') local isReclaimable = TableFind(prop.Categories, 'OBSTRUCTSBUILDING') ... if not (prop.ScriptClass == "Tree" or prop.ScriptClass == "TreeGroup") and isReclaimable then if prop.Economy and prop.Economy.ReclaimMassMax and prop.Economy.ReclaimMassMax > 0 and not isObstructing then if not isObstructing then WARN("Prop is missing 'OBSTRUCTSBUILDING' category: " .. prop.BlueprintId) end end endBecause
isReclaimableandisObstructingboth search for'OBSTRUCTSBUILDING', the condition effectively becomesisObstructing and not isObstructing, which is impossible, so the warning never fires.If the intent was to check for a separate “reclaimable” category, this should likely be:
- local isObstructing = TableFind(prop.Categories, 'OBSTRUCTSBUILDING') - local isReclaimable = TableFind(prop.Categories, 'OBSTRUCTSBUILDING') + local isObstructing = TableFind(prop.Categories, 'OBSTRUCTSBUILDING') + local isReclaimable = TableFind(prop.Categories, 'RECLAIMABLE')Please adjust to the correct category name used elsewhere in the codebase so that missing
OBSTRUCTSBUILDINGflags on reclaimable, non-tree props are actually reported.
🧹 Nitpick comments (1)
lua/system/blueprints-props.lua (1)
120-129: Tree special-case: constant cutoffs and formattingSemantically this branch does what it says: for
Tree/TreeGroupwith exactly 3 LODs you enforce[40, 165, 640]and exit early; all other trees reuse the formula but clampmaxLodto640. That’s reasonable, but:
- All 3‑LOD trees now share identical cutoffs regardless of actual size, ignoring
weightedLodSize. If your tree assets vary significantly in scale, consider at least documenting why a fixed set is preferred over size-based scaling, or reuseweightedLodSizeto derive these values.- The
if (...)andthensplit across lines (Lines 120–121) is legal Lua but non-idiomatic and was already called out by reviewers. Puttingthenon the same line as theifwould improve readability and match the rest of the file.Example formatting tweak:
- if (prop.ScriptClass == 'Tree' or prop.ScriptClass == 'TreeGroup') - then + if (prop.ScriptClass == 'Tree' or prop.ScriptClass == 'TreeGroup') then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lua/system/blueprints-props.lua(1 hunks)
🔇 Additional comments (2)
lua/system/blueprints-props.lua (2)
112-119: Size-weighted LOD metric and maxLod computation look soundUsing
MathSqrt(sx * sx + 0.5 * sy * sy + sz * sz)to deriveweightedLodSizeand basingmaxLodon that (Line 118) gives a diagonal/length-like measure with reduced Y influence and should behave well across varying prop dimensions. The math is correct and avoids pathological cases thanks to theor 1defaults on sizes.
131-139: Per-level LOD distribution and rounding behavior are consistentThe linear
(n / levels) * maxLodfactor combined withMathFloor((LODCutoff + 5) / 10) * 10correctly spaces LODs between 0 andmaxLodand rounds to the nearest 10 units for positive distances. This is clear, easy to tune viamaxLod, and avoids the off‑by‑one behavior of “always round up” schemes.
|
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. 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. |
3 participants
Description of the proposed changes
As per the title. Screenshots can be found below.
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.