Fix some glitches, caused by spritesorting

[DorpsGek]

frosch opened the ticket and wrote:

This patch fixes #777 and #820, and some things more.

Details:

1. Add support for specifying bounding boxes (BB) independent of sprite position.
   (This is needed to change the BB, when the .grf cannot be changed)
   Changes in viewport.cpp: New parameters of AddSortableSpriteToDraw()

2. Prevent BB position from overflowing. E.g. the BB of buoys on sea level had a Z-height of -256.
   Changes in viewport.cpp: Usage of max() in AddSortableSpriteToDraw()

3. Fix sprite sorter comparator test.
   Trunk uses a different sprite order comparator if the BB of two sprites overlap.
   The overlapping-test was off by one. Additionally I removed a test case, that can now never happen.
   Changes in viewport.cpp: ViewportSortParentSprites()

4. The BB of trees is fixed. They do not know, if they caused problems in trunk, but they caused problems in SmatZ's and mine SpriteSorterExperiments.
   See attached image.
   Changes in tree_cmd.cpp

5. The BB of bridges (pillars, floor, back, roof, ...) are changed. That fixes Bridge drawing glitches #777 and Pillar drawing order glich, with newbridges.  #820.
   Changes in tunnelbridge_cmd.cpp

6. The tram bits on bridges are now drawn as SpriteCombine with the floor-sprite. This fixes several cases, when the road was drawn over the tramtracks.
   Changes in tunnelbridge_cmd.cpp

I am not aware of newly introduced glitches.

If needed I can split the patch in multiple parts.

Attachments:

1. FixBridgeGlitches_r11092.patch: Well, the patch.
2. DrawBoundingBoxesWithCtrlB_r11092.patch: Test patch, that draws the BB. Toggled with CTRL-B. Most time confusing, but sometimes useful.
3. treeBB.png: Old and new BB of trees.
4. bridgeBB.png: Old and new BB of bridges: The floor is broader, the pillars extend downwards. Other changes are not easy to see.

Attachments

• FixBridgeGlitches_r11092.patch (12.85 KiB)
• DrawBoundingBoxesWithCtrlB_r11092.patch (4.24 KiB)
• treeBB.png (10.63 KiB)
• bridgeBB.png (20.78 KiB)

Reported version: trunk
Operating system: All

---

This issue was imported from FlySpray: https://bugs.openttd.org/task/1223

[DorpsGek]

frosch wrote:

First three independent parts:

PartCleanUpTownIndustryDrawTileData: (new)
- Remove stupid -1 +1, which was annoying me for months. (_xxx_draw_tile_data is only used in one place)

PartSupportBBTuning:

1. Correct Belugas' spelling :)
2. Remove unused variables: "ParentSpriteToDraw::right" and "::bottom"
3. Add compile time assertions wrt. to "LARGEST_SPRITE_LIST_STRUCT"
4. Introduce "bb_offset_?"-parameters to "AddSortableSpriteToDraw()".
   This allows specifiing the BB independent from the sprite position.
   Also make the documentation of that function more detailed.
5. Prevent the cases x_max < x_min, y_max < y_min, z_max < z_min by generously adjusting the ?_max.
   Make z_min, z_max signed.

PartFixOverlapDetectionOffByOne:

1. BBs extent from ?_min to ?max, both inclusive. I.e. Two BBs overlap even if their ???? are equal.
2. The second comparator can be simplified. The "(... && ... && ...)" has no effect: Either the first comparator is used, or the decision is already made by the "... || ... || ..." part

Attachments

• PartCleanUpTownIndustryDrawTileData_r11101.patch (2.26 KiB)
• PartSupportBBTuning_r11101.patch (7.24 KiB)
• PartFixOverlapDetectionOffByOne_r11101.patch (1.49 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2116

[DorpsGek]

Rubidium wrote:

I've committed the patches in the previous comment. What still needs to be done?

The bounding box text is nice, but it looks pretty glitchy after a while. Tip: add a MarkWholeScreenDirty() when you're toggling the flag.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2119

[DorpsGek]

Rubidium wrote:

When applying the first patch (and reverting everything except the tunnelbridge_cmd.cpp and tree_cmd.cpp code) on r11111 you still have some glitches and looks like they are even worse than without your patch).

Attachments

• fs1223.sav (87.65 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2121

[DorpsGek]

frosch wrote:

Thanks.

Here is a new version that should be better. As it is a little big, it will need some more testing.

The following BBs are altered:

1. BridgeMiddle and TramOnBridge are completely different.
2. Tunnels and elrail-catenary on tunnels are completely different.
3. Signals, elrail-pylons, tram-catenary and owned-land-markers are lower.
4. Trees are completely different.

Attachments

• NewBoundingBoxes_r11123.patch (15.36 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2135

[DorpsGek]

Rubidium wrote:

Wow, that looks way-way much better in game. However, there are two or three one pixel issues I can spot in the previous savegame at the parallel 4 bridges of which one is above tunnel entraces:
- the pilars of the bridge do flicker a very little bit when a train goes over it; it's only a single pixel
- the top pixel(s?) of the downward ramp flicker a little when a train goes over it; best seen on the southest and 3rd southest southern bridge ramps.

But I can't stress that this is looking really promising.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2139

[DorpsGek]

frosch wrote:

Fixed some glitches with tunnels and with catenary on bridges.
Improved documentation.

Known glitches:

1. Tubular-bridge-pillars glitch with tunnel-roof (see Rubidium's post above) -> It's the sprite's fault, cannot be fixed.
   Same holds for a single pixel slightly above the pillars of the tubular bridges.

2. Bridgeramps glitch with bridge-floors (see Rubidium's post above) -> Currently no idea (anymore), how to solve this.

3. Rail vehicles on horizontal/vertical track under bridges can glitch with the bridge-floor -> Similiar to Clipping problems #119. I guess: Who can fix one of them, can also fix the other.

4. Elrail-pylons and signals sometimes glitch with bridge-pillars -> I have other plans with bridge pillars. Perhaps they will fix it, but currently this is out of scope of this patch.

Attachments

• NewBoundingBoxes_r11127.patch (19.84 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2147

[DorpsGek]

Rubidium wrote:

1. Already expected that :(
   2 + 3) Oh boy, we need frosch v2 ;)
2. Have fun with that

Anyhow, I've commmitted the patch as it currently is as it solves quite a few very visible flickerings and only leaves a few very small ones.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2157

[DorpsGek]

frosch wrote:

For the interested: I have put some more work into the BoundingBox-test-patch. A lot less glitches now.

Attachments

• DrawBoundingBoxesWithCtrlB_r11132.patch (5.80 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2175

[DorpsGek]

Rubidium wrote:

Why is there such a massive change in the viewport.cpp stuff (except from the addition of that function)? What's the added functionality of that piece of code?

I think we should just add the boundingboxes drawing to trunk, but I don't exactly like the current form of the diff. Especially the big change in viewport.cpp

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2176

[DorpsGek]

frosch wrote:

AddSortableSpriteToDraw() is called for all sprites, even if they are outside the current viewport.
That codepiece discards all sprites, which do not overlap with the viewport area.
But the bounding box is most time bigger than the sprite, so if drawing bounding boxes the clipping must also consider the BB size.

I guess that piece of code can be made more beautiful (the change is only needed when _draw_bounding_boxes == true). It was only a quick hack, because I never considered it to be useful in trunk.
Why do you want to add it to trunk?

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2177

[DorpsGek]

Rubidium wrote:

There are still quite a few bugs existing wrt to the bounding boxes and it makes it (most likely) easier to debug those when this patch is applied. Having it added to trunk makes it not forgotten for people/unapplyable to their debug checkouts.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2179

[DorpsGek]

frosch wrote:

Ok, I cleaned it up a bit, and improved documentation.
_draw_bounding_boxes can now also be toggled in the title screen. Previously it was not possible to deactivate the BB drawing in the title screen, after it was enabled during a game.

Apart from #1255 glitches are now only caused by moving vehicles. MarkAllViewportsDirty() is only called with the extents of the sprite, not with the extents of the sprite and it's BB. But fixing that will need a lot of code, and I don't think it is worth it.

Attachments

• DrawBoundingBoxesWithCtrlB_r11158.patch (7.36 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223#comment2198

[DorpsGek]

Rubidium closed the ticket.

Reason for closing: Implemented

Over several revisions.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/1223