Fix the armory use distance situation #1546

necessarily-equal · 2021-10-19T21:56:37Z

Currently, there are 4 (four!) slightly different experiences in game when trying to buy stuff

"how far the bots see the armory" which is the same as "how far (using binds or commands) you might use the armory"
"the tutorial says you can open the armory with the q key"
"the hand button that hints you can open the armory appears"
"when you press q the menu opens"

This PR makes the experience nicer and make it look and feel much more consistent. I tried to avoid making a larger refactoring to avoid breaking stuff in the process (for example maybe some maps have activatable buttons?).

illwieckz · 2021-10-24T18:39:44Z

Do we need to make ENTITY_USE_RANGE larger? If I'm right with current value it's already usable for tricks like reloading though a wall and that's used by some mission maps that put thin indestructible glass in front of the armoury to protect it from alien players/bots while keeping it usable by human players.

necessarily-equal · 2021-10-24T18:41:35Z

It's already larger than what the UI allowed you to do before. So if the map worked before this commit, it should still work :)

…

On Sun, Oct 24 2021 at 11:39:54 -0700, Thomas Debesse ***@***.***> wrote: Do we need to make ENTITY_USE_RANGE larger? If I'm right with current value it's already usable for tricks like reloading though a wall and that's used by some mission maps that put thin indestructible glass in front of the armoury to protect it from alien players/bots while keeping it usable by human players. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

illwieckz · 2021-10-24T18:42:56Z

I mean, it may make the trick work and be abused in situation it was not expected before (like through larger walls).

src/cgame/cg_tutorial.cpp

src/cgame/cg_rocket_draw.cpp

bmorel · 2021-10-28T16:38:24Z

I also wonder why the range was increased from 64 to 96. This does not concerns me too much though, because there is currently no mission map working on unvanquished which uses illwieckz's trick (as for the bot problem, it would not be a regression since I've heard than tremulous didn't had bots).
To me, the trick of using stuff through a glass is not a good idea anyway, and I would rather see a trace here to detect collisions instead of just "oh, I see the armory, then I can certainly teleport stuff in my hands!", but this is a different problem.
I also noticed changes which prevents adding other activable buildings. Sure, originally, only humans could have some, but I would easily imagine neutral, activable buildings. Like in spacetracks, for example: this would prevent the annoying noise when on puts a building (barricade?) too close to the elevator's triggers. I see other uses for that, like having chasm's ladder trigger such a building, allowing aliens to put it back up for example.
In any case, since this is a refactoring, I see no reason to not make the code more team/buildable agnostic.
Last minor point (this really is a minor one): having this distance as a CVar would be nice. There's no need to keep this hard-coded, right?

necessarily-equal · 2021-10-28T19:55:45Z

Not checking for the ->usable field and prefering instead to only check for hard-coded value "armory" seems a regression to me. What's the motivation?

It could be considered as a regression I guess, but considering how there was already an hardcoded value and how to add a new buildable you would need to add it at several places anyway; I sligtly prefer it that way. Le 28 octobre 2021 18:30:48 GMT+02:00, "Morel Bérenger" ***@***.***> a écrit : ***@***.*** commented on this pull request.

…

> - if ( es->eType == entityType_t::ET_BUILDABLE && BG_Buildable( es->modelindex )->usable && - CG_IsOnMyTeam(*es) ) + if ( es.eType == entityType_t::ET_BUILDABLE + && es.modelindex == BA_H_ARMOURY Not checking for the `->usable` field and prefering instead to only check for hard-coded value "armory" seems a regression to me. What's the motivation? > { - //hack to prevent showing the usable buildable when you aren't carrying an energy weapon - if ( es->modelindex == BA_H_REACTOR && - ( !BG_Weapon( cg.snap->ps.weapon )->usesEnergy || - BG_Weapon( cg.snap->ps.weapon )->infiniteAmmo ) ) + if ( !IsVisible() ) No code is reachable in both visibility cases or in the opposite condition (IsVisible()). Thus, why not integrate this on previous if? Simply returning at end of block would allow to just `setProperty( "display", "none" ); cg.nearUsableBuildable = BA_NONE;` at end of function an be done with it. Plus, the way you do it, it means in some cases those values are not changed at all: is that what you want? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #1546 (review)

necessarily-equal · 2021-10-28T19:59:00Z

[Why] replacing all later ENTITY_USE_RANGE occurrences by ENTITY_USE_RANGE - 0.2?

The reason I updated the cgame code to use a slightly lower distance is to avoid slight rounding/prediction errors. Otherwise you might have cases where you think you can buy something, but the server will actually refuses to let you do it

necessarily-equal · 2021-10-28T20:01:48Z

Last minor point (this really is a minor one): having this distance as a CVar would be nice. There's no need to keep this hard-coded, right?

That is technically true. However doing that means having a cvar on server side then sending it to clients so they can have a relevant info. I think it's more work than it's worth.

bmorel · 2021-10-28T20:49:14Z

there was already an hardcoded value and how to add a new buildable you would need to add it at several places anyway

Makes sense.

the reason to use a slightly lower distance is to avoid slight rounding/prediction errors

But then, you should have that slightly smaller value only in cgame, right? I'm pretty sure you did it in both cgame and sgame?

Last minor point (this really is a minor one): having this distance as a CVar would be nice. There's no need to keep this hard-coded, right?
That is technically true. However doing that means having a cvar on server side then sending it to clients so they can have a relevant info. I think it's more work than it's worth.

Ah, right, it would need syncing. Indeed, that does not seems small enough to fit in the scope.

necessarily-equal · 2021-10-28T21:43:41Z

No code is reachable in both visibility cases or in the opposite condition (IsVisible()). Thus, why not integrate this on previous if? I kept the if like that instead of using a `SetProperty( "display", condition ? display : "none" );` because it was it like it was in the previous version of the code. The reason for that may very well be performance. But then, you should have that slightly smaller value only in cgame, right? I'm pretty sure you did it in both cgame and sgame? whoops, fixed

…

> On Thu, Oct 28 2021 at 20:49:25 +0000, Morel Bérenger ***@***.***> wrote: > there was already an hardcoded value and how to add a new buildable > you would need to add it at several places anyway > Makes sense. > the reason to use a slightly lower distance is to avoid slight > rounding/prediction errors > But then, you should have that slightly smaller value only in cgame, right? I'm pretty sure you did it in both cgame and sgame? >> Last minor point (this really is a minor one): having this distance >> as a CVar would be nice. There's no need to keep this hard-coded, >> right? >> That is technically true. However doing that means having a cvar >> on server side then sending it to clients so they can have a >> relevant info. I think it's more work than it's worth. >> Ah, right, it would need syncing. Indeed, that does not seems small enough to fit in the scope. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

bmorel · 2021-10-28T21:50:47Z

No code is reachable in both visibility cases or in the opposite condition (IsVisible()). Thus, why not integrate this on previous if?

I kept the if like that instead of using a SetProperty( "display", condition ? display : "none" ); because it was it like it was in the previous version of the code. The reason for that may very well be performance.

I see 0 cases in which a branch cost more than a "logical and" (&&). Previous code had duplicated code in that area (from what I've read in the diff), your patch fixed it, so you could also simplify that.

necessarily-equal · 2021-10-28T21:57:57Z

I see 0 cases in which a branch cost more than a "logical and" (&&). Previous code had duplicated code in that area (from what I've read in the diff), your patch fixed it, so you could also simplify that.

Changing the display css property may very well trigger a reflow and cause the html rendering engine to recalc, resize and redraw every element of the UI. That would be bad.

bmorel · 2021-10-29T07:17:06Z

I see 0 cases in which a branch cost more than a "logical and" (&&). Previous code had duplicated code in that area (from what I've read in the diff), your patch fixed it, so you could also simplify that.

Changing the display css property may very well trigger a reflow and cause the html rendering engine to recalc, resize and redraw every element of the UI. That would be bad.

I don't see where the flow would be changed? The last && !IsVisible() would still only be called if all previous elements are true.

PS: could you do the quoting properly?

necessarily-equal · 2021-10-31T00:03:47Z

(by @illwieckz)

I mean, it may make the trick work and be abused in situation it was not expected before (like through larger walls).

If it's any consolation, in such case, the previous behavior would actually let pr0 players who had binds buy gear anyway. I can change the distance back if it helps though. This is more practical though IMO.

necessarily-equal · 2021-11-06T23:12:43Z

ping @bmorel

src/sgame/sg_active.cpp

bmorel · 2021-12-12T22:42:44Z

src/shared/bg_gameplay.h

@@ -307,8 +307,7 @@ extern int   MEDKIT_STARTUP_SPEED;

 #define QU_TO_METER                        0.03125 // in m/qu

-#define ENTITY_USE_RANGE                   64.0f
-#define ENTITY_BUY_RANGE                   128.0f
+#define ENTITY_USE_RANGE                   96.0f


I'm not that fond of elarging the area in which one can enable armory honesty. CUrren dist seems fine to me.

src/cgame/cg_rocket_draw.cpp

bmorel · 2021-12-30T22:04:30Z

Ping. Are there still stuff to review on this?

also use an average of the two distances for the new use distance, it feels nice enough

necessarily-equal force-pushed the armory-use-distance branch 2 times, most recently from 70cb809 to a42fd43 Compare October 19, 2021 21:59

bmorel reviewed Oct 28, 2021

View reviewed changes

src/cgame/cg_tutorial.cpp Show resolved Hide resolved

bmorel reviewed Oct 28, 2021

View reviewed changes

src/cgame/cg_rocket_draw.cpp Show resolved Hide resolved

src/cgame/cg_rocket_draw.cpp Show resolved Hide resolved

necessarily-equal force-pushed the armory-use-distance branch from a42fd43 to 8bf6197 Compare October 28, 2021 22:18

bmorel mentioned this pull request Oct 28, 2021

Armory problems #1467

Closed

necessarily-equal force-pushed the armory-use-distance branch from 8bf6197 to efe9a53 Compare October 30, 2021 23:59

necessarily-equal mentioned this pull request Dec 12, 2021

armory menu can be triggered when buildable has been destroyed #1632

Closed

bmorel reviewed Dec 12, 2021

View reviewed changes

Have cgame use the shared entity use distance

7e24f9f

necessarily-equal force-pushed the armory-use-distance branch from efe9a53 to f72ed1c Compare January 11, 2022 02:09

necessarily-equal added 3 commits January 11, 2022 03:09

Unify the buildable use distance in sgame

c3f869f

also use an average of the two distances for the new use distance, it feels nice enough

Show use button on the right distance

df7497e

Have cgame use something consistent as ENTITY_USE_RANGE

9ea72fd

necessarily-equal force-pushed the armory-use-distance branch from f72ed1c to 9ea72fd Compare January 11, 2022 02:11

necessarily-equal merged commit 69e70eb into Unvanquished:master Jan 11, 2022

necessarily-equal deleted the armory-use-distance branch January 11, 2022 13:51

slipher mentioned this pull request Feb 19, 2022

double ENTITY_USE_RANGE #1759

Merged

Fix the armory use distance situation #1546

Fix the armory use distance situation #1546

necessarily-equal commented Oct 19, 2021

illwieckz commented Oct 24, 2021

necessarily-equal commented Oct 24, 2021 via email

illwieckz commented Oct 24, 2021 •

edited

bmorel commented Oct 28, 2021

necessarily-equal commented Oct 28, 2021 via email

necessarily-equal commented Oct 28, 2021 via email

necessarily-equal commented Oct 28, 2021 via email

bmorel commented Oct 28, 2021

necessarily-equal commented Oct 28, 2021 via email

bmorel commented Oct 28, 2021

necessarily-equal commented Oct 28, 2021 via email

bmorel commented Oct 29, 2021

necessarily-equal commented Oct 31, 2021

necessarily-equal commented Nov 6, 2021

bmorel Dec 12, 2021

bmorel commented Dec 30, 2021

Fix the armory use distance situation #1546

Fix the armory use distance situation #1546

Conversation

necessarily-equal commented Oct 19, 2021

illwieckz commented Oct 24, 2021

necessarily-equal commented Oct 24, 2021 via email

illwieckz commented Oct 24, 2021 • edited

bmorel commented Oct 28, 2021

necessarily-equal commented Oct 28, 2021 via email

necessarily-equal commented Oct 28, 2021 via email

necessarily-equal commented Oct 28, 2021 via email

bmorel commented Oct 28, 2021

necessarily-equal commented Oct 28, 2021 via email

bmorel commented Oct 28, 2021

necessarily-equal commented Oct 28, 2021 via email

bmorel commented Oct 29, 2021

necessarily-equal commented Oct 31, 2021

necessarily-equal commented Nov 6, 2021

bmorel Dec 12, 2021

Choose a reason for hiding this comment

bmorel commented Dec 30, 2021

illwieckz commented Oct 24, 2021 •

edited