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

WIP: Move most skeleton stuff to engine #1489

Draft
wants to merge 1 commit into
base: for-0.56.0/sync
Choose a base branch
from

Conversation

VReaperV
Copy link
Contributor

@VReaperV VReaperV commented Jan 5, 2025

Cgame-side pr: Unvanquished/Unvanquished#3276.

A proper solution to the issue of highly inefficient IPC with skeletal models. Skeleton will now be built and blended in the engine instead of cgame, and it will no longer be transferred through IPC at all, +this will avoid a bunch of extraneous copies. An additional benefit is that this will skip doing anything with skeletons for entities that get culled away. Cgame will now only send the relevant frame numbers/lerp/etc through IPC as part refEntity_t.

The layout I'm using for testing is this: https://users.unvanquished.net/~reaper/maps/layouts/plat23/test.dat.

@VReaperV VReaperV added T-Improvement Improvement for an existing feature A-Renderer T-Cleanup T-Performance labels Jan 5, 2025
@VReaperV VReaperV changed the base branch from master to for-0.56.0/sync January 5, 2025 23:30
@illwieckz
Copy link
Member

illwieckz commented Jan 6, 2025

Probably related to that problem reported by CodeQL:

Thread 1 "daemon" received signal SIGSEGV, Segmentation fault.
Util::SerializeTraits<refEntity_t, void>::Read (stream=...) at src/engine/client/cg_msgdef.h:127
127				ent.boneMods = stream.Read<std::vector<BoneMod>&>();

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 7, 2025

Oh yeah, that was an unfinished change.

@VReaperV VReaperV force-pushed the reaper/engine-skeleton/sync branch from 5ae3f4d to dc1a501 Compare January 7, 2025 09:44
@illwieckz
Copy link
Member

illwieckz commented Jan 7, 2025

So, this fixe dll. I now can get it run as exe too, but with nexe (PNaCl) I get:

Warn: IPC: Unexpected end of message

@VReaperV VReaperV force-pushed the reaper/engine-skeleton/sync branch from de64f25 to 926b067 Compare January 7, 2025 18:04
@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 7, 2025

So, this fixe dll. I now can get it run as exe too, but with nexe (PNaCl) I get:

Warn: IPC: Unexpected end of message

I've added a few more fixes, can you check if you still get this error? Do note that I have pushed fixes both on this branch and on the Unvanquished one.

@illwieckz
Copy link
Member

I get the same.

Anyway, can you rebase on latest for-0.56.0/sync so I can also test with Saigo? 🙂️

@VReaperV VReaperV force-pushed the reaper/engine-skeleton/sync branch from 926b067 to 99c6824 Compare January 7, 2025 18:19
@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 7, 2025

I get the same.

Hmm, that's weird. I'd need to look at it later, there are stil la few bugs that need to be fixed.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 7, 2025

Anyway, can you rebase on latest for-0.56.0/sync so I can also test with Saigo? 🙂️

Done, although I haven't tested it yet after rebase.

@illwieckz
Copy link
Member

Thanks, the rebase was successful (I can build the game with Saigo).

I get the same error with a nexe built with Saigo.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 7, 2025

Maybe there's padding being inserted into BoneMod in either daemon or saigo/nacl game, but not the other... At least that's the only potential explanation I have right now.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 7, 2025

This is mostly done now, I believe the only remaining issues are because trap_R_LerpTag() needs to be modified to just work from the engine instead, +fixing the legs skeletons. Well, and whatever the above error is.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 8, 2025

So I've unfortunately run into another issue: CG_SetAttachmentTag() (which is used for particle systems and such) requires some of the skeletons to already be built.

There are 3 options I can see right now: add more IPC calls to get skeletons just for those entities (which is really ugly), go back to the implementation that simply batches skeletons, or move particle systems to engine. The latter might also solve some performance issues with particle systems that we have right now.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 8, 2025

This version is, however, faster than #1386.

@VReaperV VReaperV force-pushed the reaper/engine-skeleton/sync branch from cf47328 to 06b3c18 Compare January 8, 2025 10:04
@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 8, 2025

This fixes a few things. The problems still remaining are legs skeletons not being set to torso skeletons, some animations missing/incorrect, and the above problem about particle systems/trail systems attachments.

@VReaperV
Copy link
Contributor Author

VReaperV commented Jan 9, 2025

So I've unfortunately run into another issue: CG_SetAttachmentTag() (which is used for particle systems and such) requires some of the skeletons to already be built.

There are 3 options I can see right now: add more IPC calls to get skeletons just for those entities (which is really ugly), go back to the implementation that simply batches skeletons, or move particle systems to engine. The latter might also solve some performance issues with particle systems that we have right now.

I figured out another solution for this:

  1. Add all non-particle and non-trail entities.
  2. Update their positions/skeletons etc. with an IPC call, but don't render them yet.
  3. Write the positions of bones (for tag attachments) that we're interested in with the command buffer.
  4. Read it on cgame side, and use those positions to spawn particles/trails.
  5. Add the particle and trail entities and render everything as normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants