Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r_smp is broken with portals #1216

Open
slipher opened this issue Jul 6, 2024 · 7 comments
Open

r_smp is broken with portals #1216

slipher opened this issue Jul 6, 2024 · 7 comments

Comments

@slipher
Copy link
Member

slipher commented Jul 6, 2024

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
Copy link
Member Author

slipher commented Jul 6, 2024

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
Copy link
Contributor

VReaperV commented Jul 6, 2024

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
Copy link
Member Author

slipher commented Jul 6, 2024

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
Copy link
Member Author

slipher commented Jul 7, 2024

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

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

@illwieckz illwieckz moved this to Todo in Portals Jul 12, 2024
@VReaperV
Copy link
Contributor

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
Copy link
Member Author

slipher commented Sep 9, 2024

#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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

2 participants