DaemonEngine / Daemon

The Dæmon game engine. With some bits of ioq3 and XreaL.
https://unvanquished.net
BSD 3-Clause "New" or "Revised" License
306 stars 60 forks source link

r_smp is broken with portals #1216

Closed slipher closed 5 days ago

slipher commented 4 months ago

R_GetPortalOrientations is a render frontend function, but it writes into the tess.verts buffer which is supposed to be used by the renderer backend. Sure enough, if I launch daemon with args -set r_smp 1 -set common.framerate.max -1 +devmap test_portals, it immediately crashes when the map loads.

Note: it has always been like this; the problem is unrelated to recent portal changes.

Assuming there is no quick fix, I suggest making the USE_SMP build option disabled by default.

slipher commented 4 months ago

Assuming there is no quick fix, I suggest making the USE_SMP build option disabled by default.

I guess we could use the SyncRenderThread function as a quick fix. This would make it effectively single-threaded when a portal in view, by waiting for the backend to complete.

VReaperV commented 4 months ago

Perhaps just having some duplicate memory (just somewhere in RAM) for portal vertices would fix this. The memory required for that should be insignificant.

SurfIsOffscreen() also uses both tess.verts and tess.indexes.

slipher commented 4 months ago

Yeah the tess.verts etc. buffers should be made non-global, but it will be a big refactoring.

SurfIsOffscreen() also uses both tess.verts and tess.indexes.

But it has a fallback implementation for when r_smp is enabled that doesn't do that.

slipher commented 4 months ago

I guess we could use the SyncRenderThread function as a quick fix.

Doesn't work. I get a null tess.indexes.

VReaperV commented 4 months ago

Yeah the tess.verts etc. buffers should be made non-global, but it will be a big refactoring.

Perhaps a more specific solution will suffice for portals for now, like just put all the vertices of all portals in tr and for each surface marked as a portal have the start/end of vertex range.

slipher commented 2 months ago

1291 will prevent the crash by skipping the portal and logging a warning.

Now I understand how I could make it work with R_SyncRenderThread (like I unsuccessfully tried earlier), but it doesn't seem worthwhile since that would practically disable the multi-threading.

VReaperV commented 1 month ago

This crashes again now with for-0.55.0 since other code reads vertices there. I think a rather simpler way to fix both the issues would be to calculate the portal center and AABB once. Then those can be used for culling and for determining where the nearest portal entity is etc. Moving portals should work fine with this too, as the entity position already gets added. Rotating portals might break though, unless either OBBs are implemented (which would help with other issues as well, like the stupid entity hitboxes) or the AABB is made into a cube with side equal to the longest side of original AABB. This would also improve performance as all of this wouldn't be recomputed every time there's potentially a portal in view.