We got a bug report for QS that the changelevel triggers in Xmen TC are harder to hit than with winquake. The changelevel triggers are point entities with an mdl model set on them.

What's happening is in Fitzquake, calling setmodel() also sets the mins/maxs based on the contents of the mdl, while winquake just sets them to '-16 -16 -16' '16 16 16'.

The Xmen code was calling setmodel() as the last thing in the setup code for this entity (xmen_teleport), so they were relying on the hacky behaviour of vanilla quake to set a 32x32x32 bbox.


Anyway - just thought I would document this for future reference; I suppose there could be a sv_gameplayfix cvar to revert to Winquake compatible behaviour, but it doesn't seem very urgent - lol. 

Oh... That would explain some seriously annoying issues I had with qc in the past 

sv_gameplayfix_setmodelrealbox
if 1, uses the model's bbox. if 0, uses +/- 16 for mdls.
just using +/-16 for *.mdl means that dedicated servers need not bother loading mdls at all, note that the vanilla quakeworld server does not support loading .mdl at all.
FTE uses +/-16 purely based upon the filename extension, while uses the real size in other cases. This means that replacement content still gets a suitable size with things that were originally .bsp (where they always got the bsp's size) while .mdl entities get their +/-16, and convieniently sidesteps issues that are so problematic in DP with replacement content.
Depending upon the filename extension like that is a bit of a hack, but it does indicate the original expectation much better than anything else.

quakerally is similarly (and severely) broken by mdls not getting size +/-16. 

That would explain some seriously annoying issues I had with qc in the past

Same! I figured it out after vtos() printing sizes of misbehaving things in desperation and getting non-power-of-2 dimensions. I started always calling setsize after setmodel assuming it was another QC quirk everyone knew about but me, and not something specific to Fitzquake. 

Haha, ages ago Spike and some QuakeC modders discussed that.

I did notice that anomaly in the FitzQuake source code last year when carefully comparing versus WinQuake.

I thought about it 10 seconds, and decided no one ever complains about FitzQuake.

This may be the first time someone has spotted a behavior difference in the wild and brought to someone's attention. 

Personally I'm baffled that engines still use this style of MDL bbox handling when all the code needed to handle them properly - including full rotation support - is there in Quake 2 - https://github.com/id-Software/Quake-2/blob/master/ref_gl/gl_mesh.c#L373

It's not hugely difficult to port that to Q1 and all of these problems just go away. 

I don't QuakeC much, but I've listened to plenty of conversations of QC.

I always thought in QuakeC, you were *always* supposed to do setmodel and then setsize.

But QRally doesn't, it would seem. And perhaps above comments from Necros and Lunaran indicate they don't always do setmodel and setsize too? And other mods may have made similar mistakes.

@metslime ...

Could you describe what FitzQuake was solving with the sizing the model?

Spike advocates all engines using the -16,-16,-16 to 16,16,16 but ...

I know you *never* implemented anything without a reason.

You made one of the most conscientious and thorough engines typically having a broad set of knowledge from a great number of sources. (The change wasn't accident or an oversight, but rather to solve something).

Was the issue for place_model, misc_model, mapobject_custom type of entities? 

iiuc it fixes glquake's model culling.

on the other hand, software rendering has more complicated culling, hence how it got away with hacking the size. I ought to verify that because frankly its an assumption.

and yes, qrally is NOT nice code. it violates other obscure rules too.

using +/-16 means that the server can avoid even loading the mdl in the first place. that's the biggest advantage. :) 

I am also curious about Baker's question.

Spike mentioning culling made me think.. if the server-side bbox is the +-16 units default, but the MDL is really huge (e.g. vermis), you could have a situation where the player can see the tip of the MDL, but the entity origin is outside of the player's PVS so the server culls it (wrongly).

It does seem like the main advantage of fitz's approach is for mapobject_custom type things, so very large MDL's can be supported without the mapper having to manually input the bbox size. 

I meant client-side frustum culling, rather than serverside pvs culling. 

Look at Quake 2's alias model culling code. It's easily adaptable (the key difference is that Quake 2 stores a bbox per frame) and handles rotation better than the FitzQuake code.

If it looks weird on first encounter then I'd encourage you to cross-check with modelgen.c, specifically the code it uses to scrunch the position data down to bytes. 

If you're talking about Mod_CalcAliasBounds the purpose is to calculate the bounds for the client.

GLQuake has a bug where large models are culled incorrectly, for example a dead shambler can disappear if you turn to a certain angle even though his arm should still be on screen. So my goal was to fix that.

e->model->mins/maxs are used by client for frustum culling, and that is what my change affects. ent->v.mins/maxs are used by server for PVS culling, and that is controlled by PF_setsize which I did not modify. 

@metslime -- the change isn't setsize actually, it's setmodel. I triple checked and ... well ... it is different.

I'd like to go the Spike route of -16,-16,-16,16,16,16 since it is standard Quake default.

But if even one FitzQuake targeted single player release would act up ... *sigh*

DarkPlaces defaults to original behavior with that sv_gameplayfix_setmodelrealbox 0 ... but I think many people if they heard a "DarkPlaces don't work right" with this 5 year old release would automatically blame DarkPlaces. 

I think I see what happens now in post #691 (Xmen TC). metl didn't change PF_setmodel (except for an unrelated change to do with BSP bmodels), but when a mod doesn't call setsize() after setmodel(), the new size calculated by CalcAliasBounds "leaks" into the server ent->v.mins/maxs bbox, because PF_setmodel reads e->model->mins/maxs, and uses those to set ent->v.mins/maxs.

While it sounds like an unintentional bug, I also think you are right that changing it might break serverside PVS culling for mods that are relying on the behaviour, especially in the case of a mapobject_custom with a large MDL (quoth, AD?) 

I think the only way we can know is by asking people to test sv_gameplayfix_setmodelrealbox 0 (Quake original).

Arcane Dimensions works in DP and is tested in DP and DP uses sv_gameplayfix_setmodelrealbox 0 (Quake original) and thus far no one has *seemed* to nothing anything.

/But yeah, I'm very afraid of sv_gameplayfix_setmodelrealbox 0 since nearly all single player releases have targeted that.

You'll like my source code on that, it's basically a 2 liner. 

(*) "single player releases have targeted FitzQuake in the last 10 years." 

I never realized setmodel sets the size as a side effect and that some mods rely on that.

Sounds like the fix is to set a minimum (or just force) size of 32^3 in PF_setmodel. I guess this could introduce PVS culling bugs, but only in mods that don't call setsize afterwards? And anyway culling bugs are not as bad as gameplay functionality bugs. 

Quake 2! Quake 2! Quake 2! Quake 2!

Seriously, Quake 2 has the fix for this. It has the correct +/- 16 for the server-side stuff, and it culls small/large models correctly for the client-side stuff.

There's really no need for this discussion. Grab the Quake 2 code (https://github.com/id-Software/Quake-2/blob/master/ref_gl/gl_mesh.c#L370), adapt it for the Quake differences, and all of these problems Just Go Away. 

@mh -- you 100% this is only about culling?

What does setmodelsize do? What does QuakeC do with the mins/maxs?

Is it ever used for physics?

Spike says has Q-Rally has a *physics* issue with the FitzQuake way.

/My somewhat limited QuakeC internals is why I ask 

Clarity ...

If QRally has a physics issues with the FitzQuake way.

How do we know that there aren't single player releases designed for FitzQuake that will show physics problems if this is reversed? 

Very few single player authors do QuakeC.

Enhanced GLQuake was often used before FitzQuake 0.85 so releases from 2009 and earlier should be safe.

Preach, Tronyn, metslime, necros are among the group. I would presume that they are sticklers for setmodel and then setsize. Most of the others are multi-engine types.

Sock is also targeting DarkPlaces.

Until Preach doesn't setmodel in mapobject_custom in Quoth, using the original Quake box should be fine. 

Arcane Dimensions works in DP and is tested in DP and DP uses sv_gameplayfix_setmodelrealbox 0
DP defaults this cvar to 0 but experienced DP users set it back to 1 for compatibility with some custom maps. 

Do you know the name of any particular map that it is known to make a difference? 

Well, before I set the gameplay fixes to 1 there were several maps in which I encountered bugs with pickups stuck in walls or missing altogether. I don't remember which maps specifically, except Solarfall: at the beginning of the map, turn right then left and you arrive in a room with a green armor pickup which was missing when I first played it. Though I suspect this particular issue might be fixed with sv_gameplayfix_droptofloorstartsolid 1, not setmodelrealbox.