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_R_BatchInPVS to test if a bunch of entities are in PVS in one call #852

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented May 6, 2023

Implement trap_R_inPVSArray to test array of entities in PVS in one call.

Engine sidecar of:

@slipher slipher deleted the branch for-0.55.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:15
@illwieckz illwieckz changed the title cg_api: implement trap_R_inPVSArray to test array of entities in PVS in one call cg_api: implement trap_R_BatchInPVS to test if a bunch of entities are in PVS in one call May 6, 2024
@illwieckz
Copy link
Member Author

illwieckz commented May 6, 2024

I rewrote this using lessons learned while implementing:

The code now sends and receives the shortest vector possible for valid and active entities only.

Commits are meant to be squashed so one would probably better want to review this as a whole.

@illwieckz illwieckz added T-Improvement Improvement for an existing feature T-Performance labels May 6, 2024
Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should ultimately avoid the IPC entirely by implementing inPVS in the cgame, but this is fine for now.

LGTM to the engine side.

src/shared/client/cg_api.cpp Outdated Show resolved Hide resolved

size_t numEntities = posEntities.size();

inPVS.reserve(MAX_LOCATIONS);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it makes sense to also do a inPVS.reserve(MAX_LOCATIONS); here as inPVS is an argument?

I tried it, the code builds and run, but I don't know if that changes something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to have both reserve and resize. You can do either reserve+push_back, or resize+a[b]=c.

Copy link
Member Author

@illwieckz illwieckz May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote that part, in fact I could iterate the source array directly by using push_back for the destination, which looks cleaner.

@illwieckz illwieckz force-pushed the illwieckz/entitypvs/sync branch 2 times, most recently from 02a0300 to 5ccf637 Compare May 8, 2024 03:11
@illwieckz
Copy link
Member Author

We should ultimately avoid the IPC entirely by implementing inPVS in the cgame, but this is fine for now.

That's exactly my opinion. I would like to see this in cgame to avoid IPC entirely in the future, but this is something I can reach today and the benefit is there, so we can start with that.

src/engine/client/cg_api.h Outdated Show resolved Hide resolved
src/engine/client/cl_cgame.cpp Outdated Show resolved Hide resolved
@illwieckz illwieckz force-pushed the illwieckz/entitypvs/sync branch 3 times, most recently from e5e8f44 to 9feec82 Compare May 31, 2024 17:20
Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/shared/client/cg_api.cpp Outdated Show resolved Hide resolved
@illwieckz
Copy link
Member Author

@illwieckz illwieckz merged commit e2b89c1 into for-0.55.0/sync Jun 1, 2024
9 checks passed
@illwieckz illwieckz deleted the illwieckz/entitypvs/sync branch June 1, 2024 18:59
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