baseq3a icon indicating copy to clipboard operation
baseq3a copied to clipboard

Remove BoxOnPlaneSide inline assembly version function and their unused slow version

Open LegendaryGuard opened this issue 2 years ago • 6 comments

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.

LegendaryGuard avatar Jul 19 '23 23:07 LegendaryGuard

This function is not used in mod code

ec- avatar Jul 20 '23 09:07 ec-

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).

LegendaryGuard avatar Jul 20 '23 13:07 LegendaryGuard

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

ec- avatar Jul 20 '23 17:07 ec-

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:

  • https://github.com/search?q=repo%3Aec-%2FQuake3e%20BoxOnPlaneSide&type=code
  • https://github.com/search?q=repo%3Aec-%2FQuake3e+ClearBounds&type=code
  • https://github.com/search?q=repo%3Aec-%2FQuake3e+SetPlaneSignbits&type=code
  • https://github.com/search?q=repo%3Aec-%2FQuake3e+Q_rsqrt&type=code

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 avatar Jul 20 '23 20:07 LegendaryGuard

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

LegendaryGuard avatar Jul 21 '23 09:07 LegendaryGuard

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

LegendaryGuard avatar Jul 24 '23 08:07 LegendaryGuard