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

cg_api: implement trap_BatchGetUserCmds to fetch an array of command backups in one call #848

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

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Apr 28, 2023

Engine sidecar of:

See this thread for details about this:

This branch doesn't build, I don't know how to write the serializer for this.

I believe merging this would break engine compatibility, so we may want to make this branch target a future branch, like 0.55.0/sync.

@illwieckz illwieckz marked this pull request as draft April 28, 2023 06:45
@illwieckz illwieckz changed the title WIP: implement getcmds to fetch an array of command backups in one call (the real one) WIP: implement getcmds to fetch an array of command backups in one call Apr 28, 2023
@illwieckz illwieckz force-pushed the illwieckz/getcmds/sync branch 3 times, most recently from df3a7bf to 412fc9a Compare April 29, 2023 01:49
@illwieckz illwieckz changed the title WIP: implement getcmds to fetch an array of command backups in one call WIP: implement implement trap_GetUserCmdArray to fetch an array of command backups in one call Apr 29, 2023
@illwieckz illwieckz force-pushed the illwieckz/getcmds/sync branch 2 times, most recently from f925ab4 to a8abf82 Compare May 3, 2023 05:13
@illwieckz illwieckz changed the title WIP: implement implement trap_GetUserCmdArray to fetch an array of command backups in one call WIP: implement trap_GetUserCmdArray to fetch an array of command backups in one call May 6, 2023
@illwieckz illwieckz changed the base branch from master to 0.55.0/sync May 6, 2023 08:51
@illwieckz illwieckz changed the title WIP: implement trap_GetUserCmdArray to fetch an array of command backups in one call implement trap_GetUserCmdArray to fetch an array of command backups in one call May 6, 2023
@illwieckz illwieckz marked this pull request as ready for review May 6, 2023 09:04
@illwieckz illwieckz changed the title implement trap_GetUserCmdArray to fetch an array of command backups in one call cg_api: implement trap_GetUserCmdArray to fetch an array of command backups in one call May 6, 2023
@slipher
Copy link
Member

slipher commented May 7, 2023

Check out #854. It should be even faster by requesting only one usercmd_t per frame, and doesn't require ABI changes.

@ghost
Copy link

ghost commented Nov 7, 2023

I believe this issue might need an update since mentioned issue was closed?

@illwieckz
Copy link
Member Author

illwieckz commented Nov 7, 2023

We may still want to implement this even if we already merged a workaround that fixes the performance problem. What this can bring is to make the code simpler by calling a function once at the beginning of the frame processing, instead of calling a function in various places and in many times.

The good thing with the workaround is that it achieves the same performance without breaking engine compatibility, we may still want to refactor and clean-up the code anyway. The priority is just far more lower.

@slipher slipher deleted the branch for-0.56.0/sync December 15, 2023 21:11
@slipher slipher closed this Dec 15, 2023
@slipher slipher reopened this Dec 15, 2023
@slipher slipher changed the base branch from 0.55.0/sync to for-0.55.0/sync December 15, 2023 21:16
@illwieckz illwieckz force-pushed the illwieckz/getcmds/sync branch 2 times, most recently from 785c703 to 4b86bb0 Compare May 6, 2024 03:57
@illwieckz illwieckz changed the title cg_api: implement trap_GetUserCmdArray to fetch an array of command backups in one call cg_api: implement trap_BatchGetUserCmds to fetch an array of command backups in one call May 6, 2024
@illwieckz
Copy link
Member Author

illwieckz commented May 6, 2024

This one may be less needed since an alternative code already exists and was merged in #854:

I like the fact this implementation makes the code less convoluted on game side.

This PR makes only sense if sizeof( userCmds_t ) is smaller than the kernel buffer, because if the single call to fetch all usercmd_t per frame splits the packets, the caching code only fetching one usrcmd_t per frame would be faster.

For reference:

#define CMD_BACKUP 64

#define USERCMD_BUTTONS 16

struct usercmd_t
{
	int         serverTime;
	int         angles[ 3 ];

	signed char forwardmove, rightmove, upmove;
	dtType_t    doubleTap;

	byte        weapon;
	byte        flags;

	byte        buttons[ USERCMD_BUTTONS / 8 ];
};

using userCmds_t = std::array<usercmd_t, CMD_BACKUP>;

@illwieckz
Copy link
Member Author

illwieckz commented May 6, 2024

So, sizeof( userCmds_t ) is 1536.

@illwieckz
Copy link
Member Author

illwieckz commented May 6, 2024

Actually one advantage of this implementation is that by turning the array into a vector, one could easily make the amount of command backups fetched for prediction very easy to change by the user with a cvar.

@illwieckz illwieckz added T-Improvement Improvement for an existing feature and removed T-Feature-Request Proposed new feature labels May 6, 2024
@illwieckz
Copy link
Member Author

illwieckz commented May 6, 2024

I noticed this:

void CG_DrawActiveFrame( int serverTime, bool demoPlayback )
{
	//

	cg.currentCmdNumber = trap_GetCurrentCmdNumber();

	//

	cg.currentCmd = trap_BatchGetUserCmds( cg.userCmds );
	
	//
}

20240506-082804-000 unvanquished-orbit-trap

I guess that at some point we will be able to merge those two trap calls into a single one.

But really, those trap calls now count for less than 1%, that's very good!

This Orbit screenshot was taken while running a branch featuring all my currently implement batch trap calls.

@illwieckz
Copy link
Member Author

illwieckz commented May 6, 2024

In fact cg.currentCmdNumber is not used anymore ! We can just drop everything related to it! 🤪️

This will even divide by two the time spent in trap calls in this function! 😃️

@illwieckz
Copy link
Member Author

Yay!

20240506-084303-000 unvanquished-orbit-trap

…ackups in one call

Implement trap_GetUserCmdArray.

Also drop now useless trap_GetCurrentCmdNumber and trap_GetUserCmd.

Co-authored-by: DolceTriade <vcelestialragev@gmail.com>
@illwieckz illwieckz changed the base branch from for-0.55.0/sync to master November 4, 2024 17:17
@illwieckz illwieckz changed the base branch from master to for-0.56.0/sync November 4, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Improvement Improvement for an existing feature T-Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants