Add BotRemoteAngles and BotAngles #113

ineedbots · 2024-05-02T17:28:30Z

What does this PR do?

Add BotRemoteAngles and BotAngles builtin methods for bots

Fixes ads BotAction not throwing (allows bots to shoot akimbo weapons)

Adds remote BotAction

BotStop assigns current weapon

Thanks for the contribution! Please provide a concise description of the problem this request solves.

How does this PR change IW4x's behaviour?

Are there any breaking changes? Will any existing behaviour change?
Specify the new expected behaviour if this PR adds a new gameplay feature or alters an existing game mechanic. You may provide an image or video showing the changes.

Anything else we should know?

Add any other context about your changes here.

Did you check all the boxes?

Focus on a single fix or feature; remove any unrelated formatting or code changes
Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
Follow our coding conventions
Minimize the number of commits

… adds remote bot button, BotStop assigns current weapon

diamante0018

I think the code quality of this PR has peaked superb levels. Good Job.

Rackover

Some unsafe access of ptr needs to be fixed, otherwise looks okay 👍

Rackover · 2024-05-03T08:52:02Z

src/Components/Modules/Bots.cpp

@@ -311,6 +317,43 @@ namespace Components
 			g_botai[entref.entnum].meleeDist = static_cast<int8_t>(dist);
 			g_botai[entref.entnum].active = true;
 		});
+
+		GSC::Script::AddMethod("BotRemoteAngles", [](const Game::scr_entref_t entref) // Usage: <bot> BotRemoteAngles(<int>, <int>);


It is not immediatly obvious to me what a function called "BotRemoteAngles" does nor what its parameters are, without reading the implementation. It would be nice to document them (the function "sets" the yaw and pitch, even though none of these three word appear in the function name!)

Is there a reason why the "remote angles" is not set at the same time as the "angles" in the other function?
The code does not inform me on why they need to be done separately and the name in the engine ("remote angle") i'm not sure what it means in this context.

Look at usercmd_t, angles are for the viewangles (aiming), and remote angles are for the pred missile.

Rackover · 2024-05-03T08:53:20Z

src/Components/Modules/Bots.cpp

+
+		GSC::Script::AddMethod("BotAngles", [](const Game::scr_entref_t entref) // Usage: <bot> BotAngles(<float>, <float>, <float>);
+		{
+			const auto* ent = GSC::Script::Scr_GetPlayerEntity(entref);


This function can return nullptr !
Please always check for nullptr before accessing the ent's members, lest a bad GSC script could instantly crash the game.

I guess, but since GSC VM wouldn't lie about the entref, it wouldn't be needed.

It also returns nullptr if it's not a player!
In general if a function can return a nullptr then it's better to make a check for the pointer being non-NULL as a general rule, otherwise unexpected inputs can create bugs that are very hard to understand for players (usually just a flavour of Fatal Error)

Hmm good catch, I hadn't actually looked at that function; I had thought that since all the other builtins had the same pattern, it was just safe to keep it consistent

Wait

No this check is unneeded. Scr_ObjectError will longjmp to the VM_Execute's setjmp.

It's true, it can never be a null pointer because of the long jumps.
Because it was never marked as no return in the definitions at functions.hpp the compiler will complain so those return nullptrs are just there for that reason.
A refactor initiative would be to use c++ exceptions like here https://git.alterware.dev/alterware/iw6-mod/src/branch/master/src/client/component/gsc/script_extension.cpp#L300

powered by this hook which uses try/catch https://git.alterware.dev/alterware/iw6-mod/src/branch/master/src/client/component/gsc/script_extension.cpp#L56

By using c++ exceptions to handle this in code it would no longer require to leave those nullptrs as the throw statement is already a no return
alternatively leave stuff as it is without any c++ stuff and either comment the behaviour of the function or mark objecterror and all other similar functions as no return

I see! :)
Indeed if the function is to never return it should be marked as noreturn and therefore those return nullptr should not be there. Additional short comments (just saying "this long jumps out, no return needed" or something like that) would be much appreciated
The GSC VM is a part of the engine I almost never worked with so I'm just doing due dilligence of checking your implem, sorry if my remarks are a bit naive.
I looked a bit in the engine and the Scr_Error does call Sys_Error which is a noreturn, however there are several conditions to calling Sys_Error (0x2040D06 OR !0x201A40F) and they're not documented on my end, I'm not sure what they are.
Do you confirm that Scr_Error will always call Sys_Error and therefore never return?
If so I will trust your word and go through with your change, no problem, but if you're not sure please double check 🙇‍♀️

https://github.com/JezuzLizard/T4SP-Server-Plugin/blob/main/src/codsrc/clientscript/cscr_vm.cpp#L114

A longjmp or exit (sys_error) will always occur, if a Scr_Error had actually continued execution, the VM would be in an inconsistent state, causing undefined behavior.

.evaluate will ALWAYS be false in ship builds, .evaluate is used for when there is a script debugger attached to the game, and it is trying to execute GSC opcodes pushed by the debugger.

Rackover · 2024-05-03T08:53:27Z

src/Components/Modules/Bots.cpp

+		GSC::Script::AddMethod("BotRemoteAngles", [](const Game::scr_entref_t entref) // Usage: <bot> BotRemoteAngles(<int>, <int>);
+		{
+			const auto* ent = GSC::Script::Scr_GetPlayerEntity(entref);
+			if (!Game::SV_IsTestClient(ent->s.number))


I had also just copy pasted the other method builtins for consistency

Rackover · 2024-05-03T08:55:12Z

src/Components/Modules/Bots.cpp

@@ -54,10 +56,11 @@ namespace Components
 		{ "sprint", Game::CMD_BUTTON_SPRINT },
 		{ "leanleft", Game::CMD_BUTTON_LEAN_LEFT },
 		{ "leanright", Game::CMD_BUTTON_LEAN_RIGHT },
-		{ "ads", Game::CMD_BUTTON_ADS },
+		{ "ads", Game::CMD_BUTTON_ADS | Game::CMD_BUTTON_THROW },


Would it be okay to map CMD_BUTTON_THROW to a new action named "throw" and then simply set both actions in GSC, therefore managing akimbo in GSC, rather than merging the commands in the client?

ads isn't an actual command in the engine; the correct name for ads would be speed, so having ads a compound of throw and speed would be okay? and have speed only the ads button bit, and throw its own

Rackover

I'm gonna check up every PR this saturday and draft a new release 💆‍♀️

ineed bots added 2 commits May 2, 2024 11:24

Add BotRemoteAngles and BotAngles, fixes ads bot button not throwing,…

d97dad0

… adds remote bot button, BotStop assigns current weapon

fix warning

c249947

diamante0018 approved these changes May 2, 2024

View reviewed changes

Rackover requested changes May 3, 2024

View reviewed changes

Rackover approved these changes May 9, 2024

View reviewed changes

Merge branch 'develop' into develop

bb7d38f

Rackover merged commit 4394864 into iw4x:develop May 14, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BotRemoteAngles and BotAngles #113

Add BotRemoteAngles and BotAngles #113

ineedbots commented May 2, 2024 •

edited

Loading

diamante0018 left a comment

Rackover left a comment

Rackover May 3, 2024

ineedbots May 3, 2024 •

edited

Loading

Rackover May 3, 2024

ineedbots May 3, 2024

Rackover May 3, 2024 •

edited

Loading

ineedbots May 3, 2024

ineedbots May 3, 2024

diamante0018 May 3, 2024

Rackover May 3, 2024

ineedbots May 3, 2024

ineedbots May 3, 2024

Rackover May 3, 2024

ineedbots May 3, 2024

Rackover May 3, 2024

ineedbots May 3, 2024

Rackover left a comment

Add BotRemoteAngles and BotAngles #113

Add BotRemoteAngles and BotAngles #113

Conversation

ineedbots commented May 2, 2024 • edited Loading

diamante0018 left a comment

Choose a reason for hiding this comment

Rackover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ineedbots May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rackover May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rackover left a comment

Choose a reason for hiding this comment

ineedbots commented May 2, 2024 •

edited

Loading

ineedbots May 3, 2024 •

edited

Loading

Rackover May 3, 2024 •

edited

Loading