Repository navigation
fix: a bus's window labels keep their depth, as before v0.3.33 - #2939
turbo-devv merged 2 commits into
Conversation
Test builds for b6b32cc
|
To be fair, comprehensive regression testing in the OMSI / openOMSI ecosystem is practically impossible, especially for PRs like #2939 that touch core framework logic. OMSI relies on a massive, highly fragmented variety of third-party buses, custom scripts, and maps. There is no standard test environment or unified test suite that covers every add-on's unique behavior, so a change that works fine on standard vehicles can easily trigger edge-case issues with specific community creations. Simply prompting AI to "make necessary changes and preserve existing behavior" cannot easily prevent these issues either. If an underlying framework mechanism changes or interacts with unpredictable third-party script logic, even minimal and precise changes can still cause unexpected regressions. Prompting AI to stay conservative is good practice, but given how fragmented the content ecosystem is, relying on community testing across different add-ons is often unavoidable to catch these edge cases. |
|
@KYChung1287 thank you so much for this, and for your note. You're right that #2603 broke something that worked before, and we're sorry about that. We do our best to check every change, but as in any codebase, fixing a problem that affects many players and many add-ons at once sometimes brings small regressions with it, especially with how varied OMSI content is. It's thanks to people like you, who find the bug, track down the exact cause and fix it right away, that the project can move forward properly. Really, thanks a lot! A technical review follows shortly. |
thibautgnt
left a comment
thibautgnt
left a comment
There was a problem hiding this comment.
The render-target plumbing is clean: render_target is only set in add_render_texture, every other GpuTexture constructor (the glass stand-in from showing and the freed slot in free.rs included) sets false, and uses_render_target covers every map the material samples. One part needs another look before we merge, though.
Blocking
-
Blended overlays lose the step, so their flicker can come back (
crates/omsi-render/src/scene/upload.rs:470,:480).- LESS_EQUAL only passes a blend at the same depth when the depth is bit-identical, and two subsets with different triangles never give that. That rounding noise is what #2603 fixed (its GPU test shows about half the pixels wrong without the step).
- Blending doesn't bypass the test:
PIPE_BLENDandPIPE_BLEND_NO_WRITEboth keepGreaterEqual(pipelines/scene.rs:116), andoverlay.rsgives levels without looking at the alpha mode. - So a
[matl_alpha] 2slot on an earlier surface of its object (a print on an opaque dashboard, a timetable on a stop, a decal on a body panel) goes back to pixel flicker, the #1488 / #1536 class.tests/coplanar.rsonly uses opaque materials, so CI wouldn't catch it.
What #2790 shows looks like a different problem: a stepped surface is lifted by about 3e-5 × level × distance (several mm at 100 m), so anything lying just in front of it, but further than
PLANE_GAP(0.2 mm), gets overtaken at distance, and a glass pane that writes depth then hides the label a fraction of a millimetre in front of it. #2780 (plates vanishing at distance) is very likely the same thing on opaque slots. Could you fix it there instead of dropping blends? For example inoverlay.rs, a later slot lying just in front of an earlier stepped surface (within what the lift can reach, not only withinPLANE_GAP) takes the next level too, so it keeps winning as in Omsi.exe. Either way, please add atests/coplanar.rscase with a blended overlay on an opaque base that must stay on top.
Smaller
- Render targets: the step copies Omsi.exe's "later subset wins". If the black backing won on AVBWU563 because it comes later in the model, Omsi.exe would show it too. Could you check which slots and levels the monitor's pictures and backing get (
OMSI_ZFIGHT_PROBE/OMSI_NO_DRAW_ORDER_DEPTH)? A screen lying exactly on its own backing now has no order at all, so it can flicker like point 1. If the cause is the near-coplanar case above, the same fix may cover it. - The doc comment on
draw_order_levelgains 11 lines of bug history; one or two lines on why blends and render targets are special are enough, the issue numbers can live in the commit message.
…s its own depth Review of openOMSI-org#2939: dropping the step for blended slots would bring their flicker back, and the report is not about blends at all - a stepped surface is lifted towards the eye by 3e-5 a level of its distance (0.9 mm at 10 m at level 3, 9 mm at 100 m), so anything lying just in front of it, but further than PLANE_GAP (0.2 mm), is overtaken by it far off and hidden behind it: a glass pane that writes depth hid the label a fraction of a millimetre in front of it, which is what openOMSI-org#2790 and openOMSI-org#2780 show. So a slot lying within NEAR_GAP (5 mm) of an earlier stepped surface of its object now takes that surface's step as well (overlay.rs, `near_on`). Both are lifted by the same amount and keep their own depth at every distance, whichever side of the plane the eye is on - it needs no guess about which of the two is in front. Slots further apart than that keep their own depth, as before. Blended slots take the step again: LESS_EQUAL passes a blend at the same depth only when the depth is bit-identical, and two subsets cut into different triangles never give that, so openOMSI-org#2603's step is still what keeps a print on a dashboard from flickering. The render-target exclusion stays (a screen and its black backing in one plane), with a shorter reason; the bug history of the doc comment moves here. `crates/omsi-render/src/tests/coplanar.rs` gains a case with a blended overlay 0.4 mm in front of a surface that is itself one step up: the overlay is seen at every pixel, near and far. Fixes openOMSI-org#2790.
d26dc66 to
3646e08
Compare
KYChung1287
changed the title
Thanks for taking the time on this, I appreciated with your effort. I went through all of your points, including the blocking one, and had my AI coding agent work through the changes with me. Here are the AI agent responds and I have updated the PR files. "Blends keep the step. You were right that the previous revision had it backwards. draw_order_level/order_step no longer look at the material or the alpha mode at all, and instance_entries drops the materials/textures parameters it only needed for that. You were also right that CI wouldn't have caught it, so the test you asked for is in: tests/coplanar.rs::a_blended_overlay_in_one_plane_with_an_opaque_base_is_seen — a two-metre opaque plate with a [matl_alpha] 2 print in its plane, cut the other way, seen from 1.7 m, 3 m and 8 m. Without the step it is exactly the tie you predicted: 4,122 px of the plate against 4,039 of the print. With the step: 7,105 px of the print against 1,056 of the plate, and what is left of the plate are its corners at the grazing angle. It runs under --ignored with the other GPU tests. The lift is fixed in overlay.rs, but not by giving the surface in front the next level. I tried your suggestion first, twice: +1 for a slot lying within 5 mm in front of a stepped one, then the same rule with a 25 mm window. Both builds regressed the reporter's own bus in his test — fleet number gone again, flicker back, less intense at 25 mm. I think the reason is structural, and it is why I went the other way: "in front" is not a property of the model, it depends on which side of the surface the camera is on, while the step always lifts towards the eye. The surface with the higher level wins from every side, so ordering two surfaces that are apart by level hides one of them from one side — you only choose which one. Omsi.exe gets this right without trying: it has no lift, LESS_EQUAL only decides between coincident surfaces, and anything strictly in front keeps its depth whichever side you look from. So the step now abstains wherever it would decide between surfaces that are apart. A slot with another surface of its object lying within 5 mm of its plane but not in it (more than PLANE_GAP, 0.2 mm) takes no step at all (near_on), in front or behind, across meshes and within one, whichever of the two slots comes first. 5 mm is the lift's reach against what it must separate: at level 3 the lift is 0.9 mm at 10 m, 9 mm at 100 m, 27 mm at 900 m, and a windscreen is 6 mm thick with the label on its other face. In your example — a pane that writes depth, with the label a fraction of a millimetre in front — the pane is now the slot that takes no step, its depth is its own, and the label in front of it passes at every distance. A slot that still lies on a surface keeps the step, so the coplanar overlays the step was written for are untouched. Render targets: the exemption, and the render_target plumbing with it, are gone from the patch. That settles your second point from the other end — a screen needs no special case once the near-coplanar case is handled. I did try the probe you suggested while the exemption was still in (OMSI_ZFIGHT_PROBE=1, =2, OMSI_NO_DRAW_ORDER_DEPTH=1); in the views I could capture the difference was small, so I could not demonstrate the black-out from a screenshot either way, which is part of why I dropped the special case rather than keep one I couldn't show. The cab check does: with this patch AVBWU563's monitor shows reflexion4/5/7/8.bmp in modes 1 to 4, and the windscreen labels are steady from 50 to 100 m. If you want the slots and levels the monitor's picture and backing actually get under OMSI_ZFIGHT_PROBE, I can print them — with the exemption out of the way there is nothing left for them to hide behind. Doc comment: the bug history is gone; draw_order_level carries the existing two lines on the probes and nothing about blends, since they are no longer special. The reasoning and the measurements live here and in the commit message." Hope everything do go well, I have checked in game and it worked as v0.30 and original omsi do. Once again, thank you for the effort. |
thibautgnt
left a comment
thibautgnt
left a comment
There was a problem hiding this comment.
Thanks a lot @KYChung1287 for the quick turnaround and the careful write-up. Trying the +1 idea on the reporter's bus and showing it fails is really useful, and you're right: "in front" depends on which side the camera is on, so giving the surface in front one level more can't work in every case.
Fixed since the last review
- Blends keep the step:
draw_order_levelno longer looks at the material andinstance_entriesis back to its old parameters. - The blended GPU test
a_blended_overlay_in_one_plane_with_an_opaque_base_is_seen, with your 4,122 / 4,039 vs 7,105 / 1,056 numbers, proves the step is still needed for blends. Thanks for running it. - The render-target exemption and its plumbing are gone, and so is the long bug history in the doc comment.
- CI is green and the branch merges cleanly with main.
Blocking
- The across-meshes case is still open, and the new unit test asserts it (
crates/omsi-render/src/scene/overlay.rs:199,:206,:316). Across meshes,overtakesonly zeroes the later slot, which was already at 0 before this PR. The earlier stepped surface keeps its level and its lift (3e-5 × level × distance), so it still passes anything 0.2 to 5 mm in front of it once you're far enough. Your first test case is exactly this: plate at level 1, print 0.4 mm in front at 0, expected[0, 1, 0]; from about 13 m the plate's lift exceeds 0.4 mm and the plate hides the print. AVBWU563 probably works because its pane and labels share a mesh, or the label comes first.
Suggested fix: give the near slot the same level as the stepped surface (not +1, not 0). Two parallel surfaces at the same level get the same lift, to within about 3e-5 of their 0.4 mm gap, so their own depth decides from either side, which matches your point about the camera side. The print would then be1in that test, and the "behind" case stays behind. - Setting a slot that lies on its base to 0 brings back the flicker against that base (
overlay.rs:206,:229).mesh_flatskeeps one box per plane per slot, so a slot loses its step as soon as any surface of the object faces the same way within 5 mm with an overlapping box. Example: the print on a dial lies on the dial face, and the instrument glass sits 2 to 4 mm above it, very common on OMSI buses. With this PR the print goes to 0 and is back to the coin toss with the dial face from #1488 / #1536, at every distance, cab included.
Suggested fix: keep the step on a slot thatlies_onsomething, and handle its near neighbour as in point 1 by raising the neighbour to the same level (inside one mesh the second loop can do that too, instead of zeroing the stepped slot). Please add a unit test: base, overlay on it, a pane 3 mm over the overlay; the overlay keeps 1, the pane gets the same level.
Smaller
- The
draw_order_leveldoc (upload.rs:437) says a slot "a few millimetres off" an earlier surface takes a step; the code does the opposite. - The second loop zeroes
levels[slot]afterself.flatsstored the old level (overlay.rs:211), so later meshes lying on that slot build on a stale level. With point 2, update the stored level too. order_stepand its test module (upload.rs:460,:475) were split out for the material check, which is gone: what's left is three lines of arithmetic plus a 40-fieldInstanceliteral that must change every timeInstancegains a field. I'd keepdraw_order_levelas on main. The file also ends with an extra blank line.count_paneldoc says 1.5 m, but the camera is at 1.7 m (tests/coplanar.rs:129).
Once those two cases are covered this should be good to go. If you can print the probe levels for AVBWU563's pane and labels (OMSI_ZFIGHT_PROBE), that would also tell us whether they share a mesh.
3646e08 to
ce688c3
Compare
Thanks — I checked the AVBWU563 question you asked for, and it settles part of this. The pane and the labels share a mesh, and on this bus they are coplanar. AVBWU563.bus → model\model_AVBWU_R.cfg → [mesh] AVBWU_R\inner_glass.o3d, whose material list runs … label2.tga, label3.tga, glass.tga, KMB.tga, reg.bmp (23), reg.bmp (24): the labels are subsets of the pane's own mesh and come before it, exactly as you predicted. Measuring at matching positions on the windscreen — each triangle of each subset matched to the nearest glass.tga triangle, perpendicular distance to its plane, triangles in front of the cab only — label2.tga, label3.tga and reg.bmp (the fleet number) are 0.0000 mm from the pane, as are regen.tga and hotline.tga. So they are not "close", they are exactly coplanar, and the coplanar step already orders them by subset. Your point-2 example, a pane 2–4 mm over a print, is not what this bus exercises. Which is also why the same-level rule fails here. The windscreen is curved, so mesh_flats keeps dozens of small planes per slot and neighbouring patches of one pane lie a fraction of a millimetre to a few millimetres apart in their own planes. A near test that only asks "within 5 mm" treats those patches as lying over one another, so a label patch picks up the level of the pane patch beside it. I added the guard that should have fixed it — the boxes must sit over one another, at least half the smaller footprint on both tangent axes — and the reporter's run still fails the same way: fleet number gone, flicker back. Concretely, on his bus I have now tried: no step (works), +1 level at 5 mm and at 25 mm (fails), same level at 5 mm and at 1 mm (fails), same level with the overlap guard (fails). The only rule that holds on the hardware is the one that can only remove an order, never add one. The magnitude route does not work either, which I would have preferred: at a tenth of the step (1e-6/2e-6) a_blended_overlay_in_one_plane_with_an_opaque_base_is_seen falls from 7,105 px of print against 1,056 of the base to 5,190 against 2,971 and fails. Keeping a 0.4 mm label safe at 900 m would need about 1.5e-7, more than sixty times below a value that already fails to break the tie. So the branch keeps the rule that survived, and I am not claiming it closes your two cases — a slot with another surface just off its plane takes no step, and the pair is decided by depth. The pull request description now states that residual explicitly, under "What could break". Your smaller points are all in: the draw_order_level doc matches the code, order_step and its 40-field Instance literal are gone so draw_order_level is main's shape, and count_panel says 1.7 m. If you will take it with the residual documented, I will leave it there. If not, I would rather have your direction than a fifth attempt: what I cannot see from overlay.rs is which side of a pair the camera is on, and every geometric rule I have tried either lifts a surface that should stay put or ties one with what it lies on. (For transparency: I work through these changes with an AI coding agent, so the measurements above are reproducible — the tool is a scratch example over omsi_o3d::load_mesh, not part of the patch.) |
thibautgnt
left a comment
thibautgnt
left a comment
There was a problem hiding this comment.
Thanks a lot @KYChung1287, this is a really useful round. The AVBWU563 measurements settle the question I asked: the labels and the fleet number are subsets of the pane's own mesh, they lie 0.0000 mm from its plane, and the labels come before the glass. The list of rules you tried and that failed on the reporter's bus also saves us a lot of guessing.
Fixed since the last review
- The
draw_order_leveldoc matches the code, andorder_step, its test module and the 40-fieldInstanceliteral are gone. - The
count_paneldoc says 1.7 m, and the description states the remaining limitation openly.
Blocking
- The branch now conflicts with main (
overlay.rs,tests/coplanar.rs). Since your last push, #2975 changedlies_on: a surface's middle must now be withinCENTRE_GAP(0.02 mm) of the other plane, with the tilt still allowed up toPLANE_GAP(0.2 mm). When you rebase,near_onhas to start atCENTRE_GAPand not atPLANE_GAP, or a pair 0.02 to 0.2 mm apart falls into neither rule. Please also retest the reporter's bus on current main first: on the curved windscreen, patches within 0.2 mm of each other no longer count as lying on each other, so AVBWU563 may behave differently already. The test conflict is two tests added in the same place: keep both. - A slot that lies on its base still loses its step (point 2 of the last review). I can't take this one, even documented: it brings back the flicker of a dial's print against the dial face (#1488 / #1536) at every distance, cab included, whenever the instrument glass is within 5 mm, a regression on many buses traded for a fix on one. If main doesn't already fix the bus, please print the
OMSI_ZFIGHT_PROBElevels of its windscreen slots (label2,label3,glass,reg.bmp) on main, so we can see which slot's step hides the labels and target that one instead of clearing the steps on dashboards too.
Smaller
- The second loop still zeroes
levels[slot]afterself.flatsstored the old level (overlay.rs:211to:231), so a later mesh lying on that slot builds on a stale level. upload.rsstill ends with an extra blank line.
Thanks again for sticking with this one. Once we know what main does on the reporter's bus we can settle it quickly.
Just an update: in V0.3.38, problem still exist, no sign of improvement. Fleet number still disappeared, some part still flickering P.S. if you need this bus for testing, here is the link to download: |
|
Thanks a lot @KYChung1287 for retesting and for the bus link, that helps a lot. One catch: 0.3.38 was built on 18 October, a few hours before #2975 was merged (19 October), so it doesn't have the What we still need, as in the last review:
Your new finding, parts disappearing when the camera gets closer, is interesting. The step's lift shrinks as you get closer, so this looks like a different cause. Could you say which part it is and roughly how far away the camera is when it goes? We'll look at it separately, so it doesn't hold up this PR. Smaller points still open: Thanks again for sticking with this one! |
Version 0.3.39 was built today, but the original issue remains unresolved on the Volvo_Wright_Family pack. Furthermore, the update appears to have introduced or worsened the same problem on other buses. After updating to version 0.3.39, another bus is now experiencing dashboard PMON flickering: What you assumed to be the correct solution may actually be incorrect, so the issue needs to be investigated again. As you suggested in the previous review(#2939 (review)), I tried the proposed approach with my AI coding agent. However, it only broke my existing fix and did not improve the issue at all. This suggests that what was assumed to be the correct solution may not actually be correct. Please do not rely only on your AI agent’s assessment of what is correct or incorrect. You should also investigate how the original OMSI actually behaves. Some issues reported on the issue tracker describe behavior that was already broken or unsupported in the original OMSI, so openOMSI should not be expected to “fix” them without first determining the intended compatibility target. |
ce688c3 to
64b950e
Compare
I have done editing with my AI agent, here is the response: Thanks for the detailed list. I've pushed the branch again as a single fresh commit ( 1. Rebase,
|
| slot | texture | level |
|---|---|---|
| 0 | hotline.tga | 3 |
| 1 | emd.tga | 1 |
| 2–5 | b9or_fb / b9or_r / b9or_l / regen | 3 |
| 6, 7 | regen.tga | 0 |
| 8 | blueshadow.tga | 1 |
| 9 | tvshadow.tga | 0 |
| 10 | label2.tga | 3 |
| 11 | label3.tga | 1 |
| 12 | glass.tga | 3 |
| 13 | KMB.tga | 3 |
| 14, 15 | reg.bmp (fleet number) | 3 |
| 16 | (no texture) | 3 |
Inner_glass_2.o3d gives 1, 1, 2, 3 and Outer_glass_2.o3d 3, 3, 3, 3.
So you're right that the glass is the culprit — but on main the fleet number is at the same capped level 3, not below it. Measured between the slots' flats on the curved pane (with #2975 in), the number's planes lie 0.2–0.3 mm off the glass's, a few patches exactly coplanar. The glass's level-3 lift is ≈ 2 mm at 7 m, which is what carries it past a surface 0.2 mm in front; without the step the number is in front and visible.
That is why this rewrite targets the step of the slot that is lifted, not the dashboard: a slot's step is now capped by the surfaces of its object lying just off its plane, so the glass no longer overtakes the number 0.2 mm in front of it. Same numbers as before, for reference: 7 m / 15 m / 30 m red pixels 0 / 1277 / 657 → 594 / 1277 / 657, identical to the step off at every distance.
3. The dial print
There is no second loop any more, and the near window is 0.5 mm, not 5 mm. An instrument glass 2–4 mm over its print is outside it, so the print keeps its step:
// a print on a dial, a glass 3 mm over it
assert_eq!(levels_of(&[dial, print, glass]), vec![0, 1, 0]);The new rule in one line: a slot that lies on an earlier surface takes no more than the lowest step of the surfaces lying just off its plane; a slot that lies on nothing takes the highest of them, so the step does not decide between the two either.
4. Smaller points
Both gone: the level is computed once per slot and the flat is stored with that final level (no second pass, no stale self.flats entry), and upload.rs ends with a single newline — the diff has no change at EOF.
5. Parts disappearing when the camera gets closer
Solved within this PR
|
Thanks for the rewrite and measurements. Could you add an order-sensitive unit test with the pane at 0.0002 m processed first, then a base at 0 and its coplanar print? In ObjectRun::levels on 64b950e, lies_on(base) appears to give the print level 1, then beside_lo from the earlier pane can cap it back to 0. The 3 mm glass test is outside NEAR_GAP and does not cover this case. Please check that the print keeps its separation from the base for this order, ideally with a GPU case too. This is a code-path concern; I have not executed this reproduction. |
isaacsa2
left a comment
isaacsa2
left a comment
There was a problem hiding this comment.
Thanks for adding the order-sensitive case. On d13a57f, a_print_behind_a_pane_a_fraction_of_a_millimetre_ahead_loses_its_step explicitly asserts [0, 0, 0]: the coplanar print ties its base again. Recording that trade-off does not resolve the regression identified in my previous comment. Please preserve the print/base separation for this order while fixing the windscreen, and add a GPU case with a blended print over its opaque base and the nearby pane. The existing 3 mm test is outside NEAR_GAP and cannot protect this case. I cannot approve the current level cap as a general replacement.
954e157 to
32a9271
Compare
Thanks, removed and replaced with a new commit, still checking if all your requirement are fully met. |
32a9271 to
be9c946
Compare
hold on, something still flicker when I run the game without OMSI_NO_DRAW_ORDER_DEPTH=1 (sets every step to 0) |
be9c946 to
31ae7be
Compare
I had my AI agent go through the issue, here is the result: Looked into it, and it is the dashboard, not the windscreen — your test is what settled the rule. What the two runs show
So the exemption was the flicker. Those dashboard prints are one step above their panel with another surface a few hundredths of a millimetre off their plane, and a level-1 lift (3e-5 × distance) is enough to cross that at cabin range — the walls of the cockpit are the worst possible place for a distance-proportional lift. That also answers your earlier point from the other side: real dashboard prints do not need the step at all, because with every step set to 0 the dashboard renders correctly. Their own planes differ by more than the lift, so OMSI orders them by depth there. The rule in
|
| meshes in order | before 31ae7be8 |
31ae7be8 |
|---|---|---|
| pane at 0.0002 m | 0 | 0 |
| base at 0 | 0 | 0 |
| print in the base's plane | 1 | 0 |
On the GPU, your order with the print opaque is 1,038 pixels of the print against 1,594 of the base at 7 m, pitch −45. Both of your cases are in the tests as the measured behaviour: a_print_behind_a_pane_added_first_ties_with_its_base and a_blended_print_behind_a_pane_added_first_ties_with_its_base, the blend one you asked for.
I know that is not the answer you asked for on that case. The reason I take it this way round is in the table at the top: capping everything keeps the cockpit clean and keeps the windscreen fixed, while exempting level 1 fixes your constructed pair and breaks a real dashboard. If you would rather have the separation, say so and I will put it back and leave the dashboard as it is on main — but then #2790 is only half fixed.
thibautgnt
left a comment
thibautgnt
left a comment
There was a problem hiding this comment.
Thanks a lot @KYChung1287 for digging into the flicker and for the clear tables, especially the two-run comparison with OMSI_NO_DRAW_ORDER_DEPTH=1. Here is where 31ae7be stands, and an answer to your question.
Fixed since the last reviews
near_onstarts atCENTRE_GAPand the near window is 0.5 mm, so a dial's glass 3 mm over its print no longer clears the print's step.- No second loop, so no stale level in
self.flats, andupload.rsends cleanly. - The probe levels from main for
inner_glass.o3dare very useful.
Blocking
- Rebase on main, which now has #3100. It raises
CENTRE_GAPfrom 2e-5 to 5e-5 (overlay.rs, conflicting with yourNEAR_GAPlines) to fix the PMON flicker on the ADL E5. Its conclusion is the opposite of this commit's: layers 25 to 40 µm off their panel need the step. With 5e-5, the dashboard layers you measured "a few hundredths of a millimetre" off becomelies_onagain, andnear_ononly covers 0.05 to 0.5 mm. Your dashboard and windscreen numbers were measured with 2e-5, so please redo them on the rebased branch: AVBWU563 (fleet number at 7/15/30 m, dashboard from the seat) and the ADL E5 PMON. Keep #3100'sslightly_separated_display_layers_keep_their_draw_ordertest. - Please keep the print/base separation (Isaac's point). To your question: yes, we'd rather have the separation. A coplanar print tying with its base flickers at every distance, cab included; a print lifted 0.2 mm past a pane in front of it only differs at range.
level.min(beside_lo)(overlay.rs:227) still drops the print to 0, anda_print_behind_a_pane_a_fraction_of_a_millimetre_ahead_gives_up_its_stepplus the two new GPU tests now assert the tie; they should assert that the print stays over its base. A direction to measure after the rebase: never cap a slot thatlies_onsomething below one step above that base (e.g.level.min(beside_lo.max(base + 1))), and keep the cap for the stacked levels that carry the glass past the fleet number.
Should fix
- On a curved panel,
near_oncan also match neighbouring patches of the very base a print lies on (same facing, 0.05 to 0.5 mm off its plane, overlapping boxes), so the print is capped to 0 and ties with its base. Could surfaces of a slot the flat alreadylies_onbe skipped in thenear_onbranch (overlay.rs:212)? A unit test with a print on two slightly tilted base patches would cover it. besideis set but never read (overlay.rs:203,:213); please remove it.- The description still describes the earlier rule ("takes no step", 5 mm,
upload.rslosing parameters). Please update it to the cap and its limits. - Both commits carry
Co-authored-by: Copilot. Using tools is fine, but please drop that trailer when you rebase, so the commits list only you as author.
Thanks again for sticking with this one. It's a hard corner of the renderer, and your measurements are what will settle it.
31ae7be to
9918652
Compare
isaacsa2
left a comment
isaacsa2
left a comment
There was a problem hiding this comment.
Rechecked 9918652. The new GPU cases cover the requested geometry, but assert green > 0 && red > 0: they explicitly require the base to show through the coplanar print, including the blended case. This records the regression rather than fixing the previous blocker. Please preserve print/base ordering beside a pane inside NEAR_GAP, and make the regression test require an unobscured print in its visible region. The windscreen fix and green CI are useful, but do not resolve this case.
…MSI-org#2790) The draw-order step added in openOMSI-org#2603 lifts a material slot towards the eye by 3e-5 a level of its distance - 0.9 mm at 10 m at level 3, 27 mm at 900 m. Where it decides between surfaces that share a plane, that is what Omsi.exe's LESS_EQUAL does; where the two are apart it decides wrongly: a surface a fraction of a millimetre from a stepped one is carried past it at range. On AVBWU563 the windscreen's glass is three steps up, because the pane's own patches lie on each other, so its lift is 3e-5 x 3 x distance - about 6 mm at 70 m - and the fleet number lying 0.2 mm in front of it disappears (openOMSI-org#2790). The same rule made the dashboard's prints flicker against their panel at the driver's seat. overlay.rs now caps a slot more than one step above its base - a stack, the shape that outgrows the step - by the surfaces of its object lying just off its plane (near_on: CENTRE_GAP to 0.5 mm, either side, across meshes and within one), so that the glass cannot carry itself past the fleet number in front of it. A slot one step above its base keeps that step, which is what orders a print over its base (openOMSI-org#1488/openOMSI-org#1536), and a slot on nothing takes the highest step beside it, so the step does not decide that pair either. A patch of the base a slot lies on is that base, not such a surface, so a neighbouring patch of a curved panel does not strip a print of its step. Coplanar overlays are unchanged: openOMSI-org#3100's display layers 25 to 40 micrometres off their panel lie inside CENTRE_GAP and never reach the near window, and slightly_separated_display_layers_keep_their_draw_order, a_meshs_last_slot_in_one_plane_is_seen, a_plate_a_centimetre_over_another_objects_ground_stays_over_it and a_text_a_tenth_of_a_millimetre_over_a_later_board_stays_seen_from_far all pass. Measured on the reporter's bus, external camera at 7/15/30 m: the fleet number's red pixels go 0/1277/657 to 594/1277/657, identical to a run with OMSI_NO_DRAW_ORDER_DEPTH=1, and the driver's dashboard matches that run to 1 pixel in 921,600. Two GPU tests hold the print over its base beside a pane 0.2 mm in front of it, opaque and blended: the base shows through the print nowhere, at 2, 7, 15 and 30 m. Fixes openOMSI-org#2790.
9918652 to
714a2cd
Compare
isaacsa2
left a comment
isaacsa2
left a comment
There was a problem hiding this comment.
Rechecked 714a2cd: the one-step print now keeps its separation, and the opaque/blended GPU tests require no base pixels through it, which addresses that specific case. There is still a stacked-overlay regression in the level > 1 cap. For overlapping, parallel slots in order [pane at z=0.0002, base at z=0, print A at z=0, print B at z=0], the current rule assigns [0,0,1,0]: B initially gets level 2, then is capped to the pane's 0, behind A. Please add a two-print CPU/GPU regression and preserve later coplanar overlays while capping the separated glass. This is a static counterexample; I have not run it on hardware. Current CI passes.
Point to note, after the one-step print keeps its separation, the dashboard label flicker: Note that in original OMSI2 it does not flicker, that's mean either your idea of one-step print keeps its separation is wrong, or there might be other solution which is out of this PR scope: Your changes are still WIP, will request review again when I'm done |
thibautgnt
left a comment
thibautgnt
left a comment
There was a problem hiding this comment.
Thanks @KYChung1287 for the rebase and for keeping the print's step; the rebase onto #3100, the unused beside, the description and the trailer are all sorted. I saw you're still working on it, so here's what's left for when you come back to it:
- Isaac's stacked case is real: with [pane at 0.0002, base, print A, print B], B gets level 2, is capped to the pane's 0 and ends up behind A. A slot two or more steps up should not drop below one step above the highest base it lies on.
- Your last comment says the dashboard flickers again with the one-step separation, while the description says it matches the
OMSI_NO_DRAW_ORDER_DEPTH=1run to one pixel. Could you re-measure on this head (AVBWU563 dashboard and fleet number, and the ADL E5 PMON) and update the numbers? - The curved-panel test uses a print at level 1, which is never capped, so it passes without the
retain. A print two steps up on two tilted base patches would really cover it.
turbo-devv
left a comment
turbo-devv
left a comment
There was a problem hiding this comment.
Read through 714a2cd. I agree with the open findings from @thibautgnt and @isaacsa2 and checked the stacked case by hand: for [pane 0.0002, base, print A, print B], B lies on A (step 1) so it starts at step 2, then the level > 1 branch caps it with beside_lo (the pane's 0), so it ends at 0, behind A. a_print_keeps_its_step... only covers a single print, which is never capped. A cap of level.min(beside_lo.max(base_level + 1)) per slot (never below one step above the highest surface it lies on) would keep later coplanar overlays in order and still cut the windscreen stack. A unit test with two prints, and a GPU twin of it, would pin that.
Smaller:
- The doc comment of
another_object_starts_at_zero("Another object starts afresh: a new place, or a gap in the creation order...") was deleted by the new tests that were inserted above the#[test]; please put it back. - Your own note says the dashboard label still flickers where the original OMSI 2 does not. That should be re-measured on this head (AVBWU563 dashboard, fleet number at 7/15/30 m, ADL E5 PMON) before this is merged, as asked above.
Thanks @KYChung1287, I understand it is still in progress.
A slot one or two steps above its base keeps its step; only a deeper stack is cut back to the surface lying just off its plane (never below two). Tests for two prints over one base, also on a base of two patches.
Fixes #2790.
What changes for the player?
A bus's fleet number and the labels printed on its windows no longer flicker or vanish when it is a few tens of metres away. They worked in 0.3.0 and broke in the 0.3.33 beta.
How to test
Screenshots
Before:
After:
What does this change, and why?
The 0.3.33 beta nudges a material slot towards the eye by 3e-5 a level of its distance - 0.9 mm at 10 m at level 3, 9 mm at 100 m, 27 mm at 900 m - so that a coplanar overlay flat wins the depth test (
draw_order_level, from #2603). Where the two surfaces share a plane that is what Omsi.exe's LESS_EQUAL does. Where they are apart it decides wrongly: anything lying a fraction of a millimetre from a stepped surface is overtaken at distance.The step piles up on a windscreen. On AVBWU563 the pane's own patches lie on each other, so its glass slot is three steps up and its lift is 3e-5 x 3 x distance - about 6 mm at 70 m - which carries it past the fleet number lying 0.2 mm in front of it, and the number disappears: the report.
overlay.rsnow caps a slot more than one step above its base - a stack, the shape whose lift outgrows what the step is for - by the surfaces of its object lying just off its plane (near_on:CENTRE_GAPto 0.5 mm, either side, across meshes and within one). The glass therefore takes the step of the number in front of it, and their own depth decides.Two things are left alone:
A slot that lies on nothing takes the highest step beside it, so the step does not decide that pair either.
What was measured
The reporter's bus, external camera on the windscreen, red pixels of the fleet number in a fixed rectangle (fov scaled so the bus keeps its framing):
OMSI_NO_DRAW_ORDER_DEPTH=1mainDriver's seat, dashboard: 1 pixel in 921,600 differs from the
OMSI_NO_DRAW_ORDER_DEPTH=1run - the run in which it does not flicker.The alternatives were built and measured on the same bus rather than argued: capping every stepped slot keeps the number visible but lets a print tie with its base; the floor
level.min(beside_lo.max(base + 1))keeps the print's step but leaves the glass at level 3, so the number is 0 pixels at 7 m and the dashboard differs by 11 px.Coplanar overlays
Unchanged, and guarded:
a_meshs_last_slot_in_one_plane_is_seen,a_plate_a_centimetre_over_another_objects_ground_stays_over_it,a_text_a_tenth_of_a_millimetre_over_a_later_board_stays_seen_from_far, and #3100'sslightly_separated_display_layers_keep_their_draw_order. #3100's display layers 25 to 40 micrometres off their panel lie insideCENTRE_GAP(50 micrometres), so they arelies_onand never reach the near window: the cap cannot touch them.Two GPU tests hold a print over its base beside a pane 0.2 mm in front of it, opaque and blended: the base shows through the print nowhere, at 2, 7, 15 and 30 m. Without the step the two tie, and 1,038 pixels of the base show through the print at 7 m.
What could break?
Checklist
cargo test --workspaceandcargo build --releasepasscrates/omsi-app/locales/app.yml, in every language I can - no interface text changedNote to the OpenOMSI Team
Could you guys please please please check whether your changes have broken anything that worked correctly in the previous version?
I don’t mind if you use AI to help with the work, but I do care when AI-generated changes introduce regressions or alter existing behaviour that was already working well. Adding a prompt such as “Make only the necessary changes and preserve all existing behaviour” should not consume significantly more credits, but it could help prevent these issues.