Add Model Scale Support

[JosiahJack]

Add support for models to use scale field. Used to scale up or down misc_model decorations such as statues or trees to add variety and emphasis.

This is scaling the model drawing and associated vis bounds and does not affect collisions.

[sezero]

@ericwa: Please review / test.

[JosiahJack]

I fixed some errors that were found in my sister PR to ironwail that were also present here. This should now properly respect fractional values for scale from 2/16 up to 256/16 since it's sent as a byte with 16u = 1.0f. Worked well in my test map a range of misc_models with various scales.

[JosiahJack]

Ok, on top of addressing the other comments (so far), I made one hopefully last addition to account for the bounds. This seems to be working well to prevent large models dissappearing when at the edges of the player's view. Let me know if I applied this in the wrong location.

As a somewhat related note, I don't think we should worry about affecting the collisions, however. Scaling is primarily used for decorative objects (e.g. func_illusionary hacks, misc_model, mapobect_custom). I believe it should be up to the modder to call setsize appropriately for traces and such, and up to the mapper to clip/skip-func appropriately.

[sezero]

OK, Can you please squash the multiple commits into a single one?

I'll wait for @ericwa's review. Having a review from @andrei-drexler would help too.

[sezero]

Tested the patch with https://github.com/Novum/vkQuake/files/9238858/scaletest.zip you posted, things seem broken:

[andrey-budko]

Not sure, but at least it helps to see scaled models

diff --git a/Quake/pr_edict.c b/Quake/pr_edict.c
index d414516..5b0c485 100644
--- a/Quake/pr_edict.c
+++ b/Quake/pr_edict.c
@@ -927,6 +927,9 @@ const char *ED_ParseEdict (const char *data, edict_t *ent)
            ent->alpha = ENTALPHA_ENCODE(Q_atof(com_token));
        //johnfitz

+       if (!strcmp(keyname, "scale"))
+           ent->scale = Q_atof(com_token) * 16.0f;
+
        key = ED_FindField (keyname);
        if (!key)
        {

[andrey-budko]

So many "16" in the patch. Does it make sense to add something like ENTALPHA_DEFAULT, ENTALPHA_ENCODE and ENTALPHA_DECODE for SCALE?

[JosiahJack]

Tested the patch... things seem broken

Fixed. PF_makestatic needed to also get the edict field. The reason it worked in vk was because that uses a common SV_BuildEntityState function rather than doing the same code twice; might be a good cleanup to replicate here later, but don't want to clutter this PR.

[JosiahJack]

...Does it make sense to add something like ENTALPHA_DEFAULT, ENTALPHA_ENCODE and ENTALPHA_DECODE for SCALE?

Good call, that cleaned it up quite a bit.

Bah, fixed a misplaced typecast. Should be good now.

[sezero]

Fixed. PF_makestatic needed to also get the edict field.

No, not fixed. Still the same result for me with ./quakespasm -game scaletest +map test_scale

How are you actually testing this yourself? (You are, aren't you?)

[JosiahJack]

No, not fixed. Still the same result for me with ./quakespasm -game scaletest +map test_scale

I'm sorry. It looks like I had copied the wrong progs.dat into the zip. I updated the download zip where I posted it originally here.

[sezero]

Took https://github.com/Novum/vkQuake/files/9245746/scaletest.zip and the latest version of the patch here, yes it seems to work. To make sure I re-tested previous b86963a65 with it and it doesn't work, so the change to PF_makestatic does seem needed.

@ericwa, @andrei-drexler, @Novum: Comments? Is this good to go?

[andrei-drexler]

I think scale also needs to be applied in R_AddEfrags (as @mhQuake mentioned), otherwise we'll get disappearing models in some cases: https://github.com/sezero/quakespasm/blob/1f6888e7bc64051c4fff3ead7f3650f04f7cecf4/Quake/gl_refrag.c#L183-L187

[JosiahJack]

I think scale also needs to be applied in R_AddEfrags...

Added.

I also noticed that r_showtris wasn't scaling here since that doesn't share the drawing code with the normal render path; this is now fixed too (duplicated over to the vkQuake PR as well, ironwail already had a shared function so didn't need this).

[andrei-drexler]

Alright, thanks! There's one more minor issue that I noticed - there's a slight inconsistency (compared to QSS) in the way very small scales are handled. The left-most model in the test map (scale 0.05) is invisible in QSS, whereas with this patch it's rendered at default scale. For consistency, it might be better to change ENTSCALE_DECODE to

#define ENTSCALE_DECODE(a)  ((float)(a) / ENTSCALE_DEFAULT) // Convert to float for rendering

While we're at it, we could also simplify ENTSCALE_ENCODE a bit (and maybe remove ENTSCALE_ZERO altogether, since it doesn't really mean anything)

#define ENTSCALE_ENCODE(a)  ((a) ? ((a) * ENTSCALE_DEFAULT) : ENTSCALE_DEFAULT) // Convert to byte

[JosiahJack]

Alright, thanks! There's one more minor issue that I noticed - there's a slight inconsistency (compared to QSS) in the way very small scales are handled. The left-most model in the test map (scale 0.05) is invisible in QSS, whereas with this patch it's rendered at default scale...

Ah good point. Updated.

[sezero]

This is in, thanks.

[sezero]

@ericwa, @andrei-drexler, @Novum: send patches if issues are discovered in future.

[sezero]

Obviously I was too hasty applying this patch: things are very broken: No models are drawn, e.g. no viewmodel no other enemies, etc. Just try playing plain id1. Please either send a patch or I'll revert this.

[sezero]

Obviously I was too hasty applying this patch: things are very broken: No models are drawn, e.g. no viewmodel no other enemies, etc. Just try playing plain id1. Please either send a patch or I'll revert this.

It looks like your last force-push (i.e. the ENTSCALE macro changes in protocol.h) broke things.

[andrei-drexler]

@sezero playing id1, it seems only the viewmodel is missing, the enemies show up just fine. Since the viewmodel is a separate entity, not networked, its scale also needs to be initialized now, since 0 actually means 0. QSS does this in CL_ClearState, but since there's no nullentitystate state here, I'd set the viewmodel scale to ENTSCALE_DEFAULT in V_CalcRefdef:

diff --git a/Quake/view.c b/Quake/view.c
index 6a24181..795c246 100644
--- a/Quake/view.c
+++ b/Quake/view.c
@@ -822,6 +822,7 @@ void V_CalcRefdef (void)
    view->model = cl.model_precache[cl.stats[STAT_WEAPON]];
    view->frame = cl.stats[STAT_WEAPONFRAME];
    view->colormap = vid.colormap;
+   view->scale = ENTSCALE_DEFAULT;

 //johnfitz -- v_gunkick
    if (v_gunkick.value == 1) //original quake kick

[sezero]

@sezero playing id1, it seems only the viewmodel is missing, the enemies show up just fine.

id1 starts with demos playing: the enemy models are missing during demo playback.

[andrei-drexler]

Ah, you're right, I had demos turned off, so I just started a new game and played through e1m1 a bit.

[andrei-drexler]

The issue is that scale isn't updated in CL_ParseUpdate for protocol 15. QSS copies baseline state before updating the fields. Proposed fix:

diff --git a/Quake/cl_parse.c b/Quake/cl_parse.c
index 565ac8c..3814b16 100644
--- a/Quake/cl_parse.c
+++ b/Quake/cl_parse.c
@@ -622,6 +622,7 @@ void CL_ParseUpdate (int bits)
        }
        else
            ent->alpha = ent->baseline.alpha;
+       ent->scale = ENTSCALE_DEFAULT;
    }
    //johnfitz

[sezero]

That, along with the V_CalcRefdef change, seems to fix things.

Any alternatives?

[andrei-drexler]

The main alternative would be to treat a byte value of 0 as default (1.0) scale, instead of actual 0, but that would deviate from QSS and FTE. If this had been a brand new feature, I would've also added some clamping, but since other ports have already had this feature for a while, I think it's best we match their behavior.

[sezero]

Applied your two patches - if there is a better solution to everything I'm all ears.

I wish we shouldn't have to hack this much around for this..

[mhQuake]

CL_EntityNum is a good place to initialize this kind of thing - other entity_t members which need initialization to non-zero are also handled there. Obviously statics and the viewmodel still need their own handling, but using CL_EntityNum should ensure consistent behaviour across all run types and protocols.

[sezero]

CL_EntityNum is a good place to initialize this kind of thing - other entity_t members which need initialization to non-zero are also handled there. Obviously statics and the viewmodel still need their own handling, but using CL_EntityNum should ensure consistent behaviour across all run types and protocols.

Can you send a patch, please?

[JosiahJack]

Sorry about these issues. Is all ok now? If so I'll get to work porting the fixes to my other PR's out for ironwail and vk to avoid these issues there, if applicable, and replicate the cleaner ENTSCALE_ values which I believe should still need done there.

Otherwise I can look into any lingering issues that persist here?

[sezero]

Seems OK so far at my end. Play (and play demos) as much as you can to make sure, though.

[temx]

I have a working version of this for vkQuake (the original patch did not handle non-FTE protocols, plus some other problems discussed here). It's worth pointing out the SV_CreateBaseline and CL_ParseBaseline changes are protocol-breaking (that is, engines not supporting this will error out when trying to play back a demo or connect to a network server which has scaled entities). Are we OK with this?

[sezero]

SV_CreateBaseline and CL_ParseBaseline changes are protocol-breaking (that is, engines not supporting this will error out when trying to play back a demo or connect to a network server which has scaled entities).

That is a problem, and I failed to recognize that.

[sezero]

SV_CreateBaseline and CL_ParseBaseline changes are protocol-breaking (that is, engines not supporting this will error out when trying to play back a demo or connect to a network server which has scaled entities).

That is a problem, and I failed to recognize that.

And without inventing another new protocol (which we have an inflation of), this doesn't seem OK.

@ericwa, @andrei-drexler, @Novum, @Shpoike: comments?

[JosiahJack]

Isn't this already restricted to just protocol 999 in the parse update? Scale already existed as a part of that protocol, it simply wasn't used.

[temx]

It's supported for normal updates on protocols 666 and 999, but not for baselines (the new field "B_SCALE" is most certainly a protocol change). Anyway, don't take my word for it: just record a demo in the test level and you'll see it doesn't play back in any other client. (The FTE protocol variants do support this since they encode baselines the same way as regular updates.)

[ericwa]

#define B_SCALE (1<<3) was originally part of protocol 999 (the protocol was developed by MH in RMQEngine https://www.quaddicted.com/files/engines/rmqengine_20110126.zip ), so I think it's correct to support that in 999 mode, and if 999 supporting clients are erroring when they encounter B_SCALE I'd say that is a client bug.

(I haven't run RMQEngine in a while, but it would be good to test recording a demo on the scale testmap in RMQEngine with sv_protocol 999, and ensure it plays back in QS with this patch. Same with recording in QS and playback in RMQEngine.)

I agree if we're running a QS server in protocol 666 and record a demo on the test level, it should play back in e.g. fitzquake 0.85.

[temx]

Hmm, that's interesting. That 20110126 build is not compatible with 999 as QuakeSpasm (or FTEQW) interprets it (for one, it doesn't send or parse sv.protocolflags, so it fails right away there).

if 999 supporting clients are erroring when they encounter B_SCALE I'd say that is a client bug

It's a very widespread bug then. FTEQW handles it, but it seems to be the only engine that does.

[ericwa]

Sorry, I must have posted the wrong RMQEngine link above - that appears to be an older iteration of 999 which lacks sv.protocolflags. I think this is the latest code: http://svn.icculus.org/remakequake/engine/Experimental/Quake/

IIRC when adding the 999 support to QS I referred to the "Experimental" branch of RMQEngine above, and I think it was just my oversight to not add B_SCALE support. So at least the QS forks missing B_SCALE can be blamed on that.

[Novum]

@temx We need to merge this into vkQuake too. I'm a bit busy with a newborn right now, but I guess it can wait.

[sezero]

Well, do we have a way of handling B_SCALE properly? If not, should we really support this scale feature at all?

[sezero]

I'm a bit busy with a newborn right now

Congrats!

[Shpoike]

@sezero 999's B_SCALE may be an issue for QS but its not one I'd worry about too much. Its not like QS even defaults to 999 right now anyway. If you MUST have a 666-with-scale then you already have that in the form of 999-with-protocol-flags-0. Ultimately remember that protocol versions are for excluding alternative interpretations rather than anything else - for things like this it becomes more of a WHO than WHAT, at least for reserved/unused bits/values. Yes, QS does not 'own' the meaning of 666 nor 999, but my point is that it doesn't need to at least for this. While it would be ambiguous for 666(and thus mustn't be used, unless metlslime okays it - until then he might claim it means something else instead) it is unambiguous for 999 (pretty much a case of 'whatever mh says goes', at least for reserved/unused bits) so there's no need for a new protocol when you can just report 999(with protocol flags 0) instead. Its not like QS has any handshakes to detect client capabilities/revisions, nor can a demo really support that anyway. There is no ambiguity here, so no need for a new protocol version.

Either way it is better for an old/buggy client to receive some weird illegible server message on the odd map that uses scale than for ALL old clients to receive more verbose errors (but still kinda meaningless to most users that are ALSO only resolved properly by updating). I might say differently if it were a mid-game error, but these are not. If you're paranoid then you can tweak the SV_CreateBaseline to not send B_SCALE, falling back on the U_SCALE bits instead. The problem will then be constrained to just makestatic (which has no alternative). Its worth nothing that DP does NOT support scale (nor even alpha) for such entities, and as makestatic() also can't be killtargetted/moved/toggled various mods are already refraining from using it (eg: AD requires an explicit spawnflag). With that baseline tweak, QS's omission would probably be insignificant enough that almost noone will notice, at least while people are still also trying to cater to DP too. You then can enable it in baselines in a few releases time or so, once more people are using a client that can handle it properly.

So yes, fix it up to not send scale for 666 - send scale ONLY with sv_protocol 999. Add errors/warnings for unknown/not-yet-defined bits so there's fewer issues in the future. Mods that want scale support will need to override sv_protocol to 999 like they already need to do for larger maps or smoother rotation.

[mhQuake]

I'm not sure why I overlooked scale in the baseline when designing this originally, other than it must have been an unintended oversight.

Suggestions:

Baseline is always ENTSCALE_DEFAULT. That needs the least amount of changes, but scaled entities will always need to send their scale, and static entities don't get to join the party.

Add baseline scale and break 999 (or create a new protocol version).

Add a new protocol flag to 999 for supporting baseline scale. 999 was designed to be extensible in this way so that would also work.

Drop the scaling feature altogether.

[temx]

A separate issue found while testing this that (IMO) must be fixed: As it is right now, scales get sent (both on baselines and on updates) in vanilla id1 (any ents for which PF_makestatic hasn't been called get a baseline scale of 0 - then updates set a value of 16). This wastes bandwidth and makes the "protocol breaking" issue always apply even if you don't use the feature at all. I'm not sure what's the best way to fix this. Adding GetEdictFieldValue in SV_CreateBaseline fixes the baselines, but stuff like the lavaballs in the hard skill hall still have their scale sent (this is less bad, but still undesirable).

[sezero]

A progs.dat compiled with .float scale; added to defs.qc is enough to generate demos from plain id1 maps that would fail with illegible server message in quakespasm-0.94.7.

[sezero]

Ah, just saw @temx's note above mentioning the same thing.

[mhQuake]

At this stage surely the best thing is to abandon RMQ scale as broken, add a new protocol flag to 999, and implement it properly?

[sezero]

I really am tending to revert the current patch, unless someone sends a foolproof fix which seems unlikely.

[temx]

A progs.dat compiled with .float scale;

I can reproduce this with the original 1.01 / 1.06 progs.dat, no need to add any field.

Related problem: existing 666/999 demos have dynamic entities missing (e.g. projectiles / dropped backpacks - anything without a baseline).