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

Remove BoxOnPlaneSide inline assembly version function and their unused slow version #27

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

LegendaryGuard
Copy link
Contributor

@LegendaryGuard LegendaryGuard commented Jul 19, 2023

On BoxOnPlaneSide function, that inline assembly code can only be compiled on MSVC compiler (Microsoft Visual C++). That has been tested on win32-msvc solution.

Previously, the code had conditional compilation directives specific to LCC, C_ONLY, id386 and __VECTORC. The refactored code provides an unified implementation that is compatible with multiple compilers.

Additionally, the commit includes a comment explaining the reason for the code change and its relationship to the Kenny Edition and also removes the commented BoxOnPlaneSide2 function being the slow version which is unoptimized.

…ility and remove commented slow version of this function
@ec-
Copy link
Owner

ec- commented Jul 20, 2023

This function is not used in mod code

@LegendaryGuard
Copy link
Contributor Author

LegendaryGuard commented Jul 20, 2023

You're right. This function has never been used in most gamecode mods. But it always has been there and in all Q3 gamecode repositories.

One year ago, in one of my mods, I've experienced an error when I tried to compile with make to get DLLs, the macro conditional blocks of this function weren't correctly handled. So, I researched what it was, the cause was because of this inline assembly code hadn't a macro conditional block that only can be compiled with MSVC.

The other one without inline assembly (which uses C), it's optimized for the most compilers (non MSVC).

@ec-
Copy link
Owner

ec- commented Jul 20, 2023

Essentially, this is a dead code from mod perspective, so the only correct solution - remove it from mod codebase, otherwise it is totally useless, there is no other 'but...' cases except increasing entropy of this repo

@LegendaryGuard
Copy link
Contributor Author

LegendaryGuard commented Jul 20, 2023

Wait.
It cannot be removed, otherwise the compiled build in-game won't work and could get some errors when playing. Because it needs to stablish the relationship to the engine. Moreover, this isn't the only function that isn't being used on gamecode, also there are others like ClearBounds, SetPlaneSignbits, Q_rsqrt, ...
That's why:

I guess I'll have to open a similar Pull Request on Quake3e about the optimization of this function too.

EDIT: Gotta remove the #if block where ignores if it's not defined Linux and BSD. It doesn't make sense.

@LegendaryGuard LegendaryGuard changed the title Refactor BoxOnPlaneSide function for improved cross-compiler compatibility and remove commented slow version of this function Refactor BoxOnPlaneSide function Jul 21, 2023
@LegendaryGuard LegendaryGuard changed the title Refactor BoxOnPlaneSide function Remove BoxOnPlaneSide inline assembly version function Jul 21, 2023
@LegendaryGuard
Copy link
Contributor Author

LegendaryGuard commented Jul 21, 2023

Ok, when you agree and are ready to merge, squash commits before merging.

@LegendaryGuard LegendaryGuard changed the title Remove BoxOnPlaneSide inline assembly version function Remove BoxOnPlaneSide inline assembly version function and the unused slow version Jul 21, 2023
@LegendaryGuard LegendaryGuard changed the title Remove BoxOnPlaneSide inline assembly version function and the unused slow version Remove BoxOnPlaneSide inline assembly version function and their unused slow version Jul 21, 2023
@LegendaryGuard
Copy link
Contributor Author

LegendaryGuard commented Jul 24, 2023

Done, now the function is the same as Quake3e engine one (the enhanced).
I hadn't checked it.
Thanks for noticing that detail.

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

Successfully merging this pull request may close these issues.

2 participants