Ctf Frogbots #365

SpookyScaryDev · 2024-09-05T19:35:24Z

Allowed KTX Frogbots to play CTF matches. There are a lot of changes here so I appreciate it will probably take a long time to get this to the point where it can be merged, but I will try and respond as quickly as possible :)

General Features / Changes

Added a new command 'botcmd gomarker' for k_fb_options 3 < 3. Bots will all abandon their current goals and run to the marker the player was looking at when they used this command. Useful for debugging movement on complex maps. This command is admin only.
Added functionality for bots to shoot buttons and doors. This has to be added explicitly in the .bot file.
- Added a new path flag LOOK_BUTTON. This marks a path from a regular marker to a button. Bots won't move along this path, it is used to show them where to look when shooting the button.
Increased NUMBER_SUBZONES from 32 to 128, since all the CTF maps have very large rooms. This is not strictly necessary, but it makes adding support to maps much easier.
Bots will abandon goals if a teammate is already going for them, except artefacts and flags.

CTF Support

Bots now play CTF! They will (attempt to) capture and return the flag and grab runes.
- Bots prioritise runes in this order, from best to worst: res, str, hst, rgn
Bots each have one of 3 roles - attack, defend, midfield
- Attackers will try and cap the flag
- Midfielders will prioritise quad
- Defenders will stay closer to the base and try to defend
- Defence points are set manually in the .bot file.
- Bots are added with roles in this order: attacker, midfield, defend, attack, midfield, etc.
- Depending on the bots role, it will ignore the distance of relevant goals when deciding a goal
Bots will prioritise enemies carrying their flag.
Bots carrying a flag won't chase enemies.
Increased bot lookahead time for CTF.
Bots can now use the grappling hook. Hook paths must be added explicitly in the .bot file.
Updated bot messages.
- Going for flag
- Base safe / not safe
- All messages include whether the bot has the flag / which rune they have.
For CTF, .bot filenames must end in _ctf to distinguish them from other modes, as the item ids are different.
On E2M2, bots on the red team will grab lg and discharge when they first spawn.
Added CTF names (this is just for fun and I can remove them if you want!)

Known Issues

These are all not too high priority as far as I can tell, and I haven't been able to figure them out so far.

team1 and team2 spawns were not automatically turned into markers. Unfortunately, I noticed this relatively late on, so all the .bot files I made have a custom marker right next to all the spawn points instead. It would be easy to fix the spawn points to make them markers automatically, but this would ruin the marker numbering in all the bot files I created and any other bot files created for maps with these spawns.
If a player adds some bots, changes teams and adds more bots, some of the bots will be the wrong colour on the scoreboard. Can't work out why this is as it works fine in 4on4.
All the movement debug logging doesn't include hook routes yet.
I had to add checks for NULL to bot_botenemy.c, line 140 due to crashes. As far as I can tell, this could have caused crashes anyway, I'm not sure if this is linked to the CTF code or not.

There are several other parts where I'm not sure if I've made good decisions or not, these are marked in my code with 'TODO hiipe', I can summarise these here if that would be preferable.

I hope that covers everything, this is my first time trying to contribute to a big project like this, so I would appreciate any feedback.

Thanks,
hiipe

dsvensson · 2024-09-05T20:22:40Z

Very nice to see it finally proposed! Currently all the CI jobs fail. You can see the output in your own repository:

https://github.com/SpookyScaryDev/ktx/actions/runs/10723857587

It would be nice if you could trim down this massive amount of commits to a bit fewer commits. No need to keep all your back and forth attempts, typos and whatnot, in this fairly isolated (from human gameplay perspective) PR. In your description you mention standalone bugfixes which are typically something nice to have in separate commit(s). Trimming down number of commits would probably also make a rebase less painful instead of the merge-with-master which likely broke the build for you. Prettier to deal with the build-breakage in the commits that introduce them during a rebase. My personal opinion is that each commit should contribute discrete value or lay the foundation for future commits to do so, and the tree should be in a functional state at each point in history. If it feels dumb to write an elaborate commit message for a specific commit, then perhaps that commit should be folded into a related commit. But that's just my own take, not the law of the project.

If going that route, I would propose you reset back to before the merge commit, then git rebase -i 934ea13bee69888940fd08b2ab78d9ff81032b1b (point of diverge) and shuffle around your commits and clean up history, and then rebase on top of upstream master. As you're 18 months behind master, you could rebase in steps to have a breather before rebasing further into the future so you don't have to deal with all the pain at once. At each step you can just push to verify that CI passes on platforms not native to you. If you stumble across some breakage, then commit that and shuffle it into the commit that introduced it with another git rebase -i $ancestor.

The conflicts and build failures ought to be fairly limited to strict-prototypes, that is, no empty () declarations for functions (which allows passing an arbitrary amount of parameters to void functions, cought a number of bugs). The other potential reason might be changes that were made to make QVM target compileable again, mostly c89isch fixes like moving declarations to start of block. Once you've cleaned up your history, it should probably be fairly quick to deal with the conflicts during the rebase.

In the future, keep feature branch duration short, and rebase often to minimize pain.

There are also a number of FIXME's and TODO's in there as you mention that ought to be resolved, or elaborated on, or removed and created issues for on merge. If merged, they will be there 10 years from now.

In addition to the CI errors, there are a number of warnings added that are valid complaints (some of them ought to be errors even, but that's OT here). Browse through them and see what cleanups you might want to do. Can probably skip looking at the macOS build, as it has so many type warnings in upstream atm, have a fix for it but will wait with that until you're done to not cause additional pain.

Will do a deeper read later.

...and for future inspiration https://x.com/GuangyuRobert/status/1831006762184646829 :)

dsvensson · 2024-09-17T19:40:41Z

@SpookyScaryDev no need to close, you can just push -f to the same branch.

dsvensson

Phew... there was an end after all took the whole train ride to read. Haven't tried running it yet. Would be nice if you could introduce some .bot files in a separate commit in the resources/example-configs/ktx/bots/maps/ dir, or if you go with the review comment, resources/example-configs/ktx/bots/maps/ctf/ dir. Overall it looks pretty good, gj!

Getting rid of accidental unrelated code changes in the last commit is trivial,

# 's' to split a hunk into smaller pieces, y/n to accept, q once you've fixed the last one to avoid hammering 'n'.
git checkout -p HEAD^ -- src/route_calc.c # sample, don't recall which files had some minor issues
git diff --cached # verify that you have staged what you intend to undo
git commit --amend

Always nice to leave blame on someone else :)

dsvensson · 2024-09-18T16:16:29Z

src/bot_items.c

+	if (player->ctf_flag & CTF_RUNE_MASK)
+	{
+		int newRune = rune->ctf_flag;
+		int currentRune = player->ctf_flag & 15;


15 -> CTF_RUNE_MASK, but could also do something like this which has established use in other parts of the code base:

int currentRune; if ((currentRune = player->ctf_flag & CTF_RUNE_MASK)) { ... }

dsvensson · 2024-09-18T16:20:38Z

src/bot_loadmap.c

@@ -172,6 +167,8 @@ static fb_spawn_t stdSpawnFunctions[] =
 	{ "door", fb_spawn_door },  // covers func_door and func_door_secret
 	{ "func_button", fb_spawn_button },
 	{ "info_player_deathmatch", fb_spawn_spawnpoint },
+	//{ "info_player_team1", fb_spawn_spawnpoint }, // TODO hiipe - these should be in here, but adding them will mean having to
+	//{ "info_player_team2", fb_spawn_spawnpoint }, // put all ctf map files through a script to correct the numbering. Later...


There's also info_player_team1_deathmatch and info_player_team1_deathmatch available to CTF when using k_ctf_based_spawn. Currently a severe lack of maps using it as it's a bit unknown. When enabled and match has started, players have a 50% chance to pick a info_player_teamX_deathmatch spawn or a info_player_deathmatch spawn. An example usecase would be to have more info_player_teamX_deathmatch near home base, and more info_player_deathmatch near mid, to weight respawns a bit towards home base rather than going all in k_ctf_based_spawn.

As for having to update the current CTF-specific e2m2.bot file etc, do that before the merge to avoid getting into a situation where more files have this problem. It's complete crap that the indices changes based on this list, but is what it is.

dsvensson · 2024-09-18T16:22:53Z

include/fb_globals.h

+#define MARKER_FLAG1_DEFEND         2048    // Point used to defend flag 1 (red)
+#define MARKER_FLAG2_DEFEND         4096    // Point used to defend flag 1 (blue)
+#define MARKER_LOOK_BUTTON          8192    // A button can be shot from this marker - set automatically
+#define MARKER_E2M2_DISCHARGE       16384   // Bots on red team will run here then discharge at the start of the map


Should probably gain a better name than MARKER_E2M2_DISCHARGE. Perhaps MARKER_MATCH_START_DISCHARGE or so unless you can think of something shorter. Might be a usecase on future or past maps even if the most common one happens to be on e2m2. Perhaps MARKER_IS_DM6_DOOR tricked you into map-specific naming.

Would be nice with MARKER_DISCHARGE_AT_START_RED, MARKER_DISCHARGE_AT_START_BLUE to make it generic for any map and team. Would require some other flag character as 'd' is taken. yay nasty char flags.

But even changing it to MARKER_DISCHARGE_AT_START_RED would be a good start at making it generic, moving maps_map_e2m2.c to bot_discharge_start.c, changing functions to DischargeAtStartLogic etc.

dsvensson · 2024-09-18T16:40:47Z

include/progs.h

@@ -419,7 +419,7 @@ typedef void (*fb_entity_funcref_t)(struct gedict_s* item);
 #define NUMBER_PATHS	8
 #endif
 #ifndef NUMBER_SUBZONES
-#define NUMBER_SUBZONES	32
+#define NUMBER_SUBZONES	128 // TODO hiipe - check this, should be ok though?


All arrays indexed by subzones uses this for size, so I can't find any reason for why this wouldn't be fine. Perhaps read through and verify yourself to remove your uncertainties, and finally the comment. It will only spread uncertainties for the next person reading it.

dsvensson · 2024-09-18T16:42:55Z

src/bot_aim.c

@@ -9,6 +9,7 @@
 #define ATTACK_RESPAWN_TIME 3

 qbool DM6FireAtDoor(gedict_t *self, vec3_t rel_pos);
+qbool E2M2DischargeLogic(gedict_t* self);


Same comment as the define name, get rid of map specific names in favor of more explanatory names. It's a generic feature that only happens to be commonly associated with e2m2.

dsvensson · 2024-09-18T18:01:31Z

src/marker_load.c


 	// Load bot definition file: frogbots rely on objects spawning 
 	//    markers, so be aware of alternative .ent files
 	char *entityFile = cvar_string("k_entityfile");
+	if (isCTF()) strlcat(entityFile, "_ctf", sizeof(entityFile));


sv_loadentfiles_dir ctf I wonder if it's too ugly to simply use that cvar for .bot files as well. It would fix the problem, and it's only ever set to something other than "" for CTF in the example configs, which are likely very similar to the nquakesv-configs that everyone uses.

dsvensson · 2024-09-18T18:02:41Z

src/route_calc.c

@@ -52,12 +52,41 @@ static void TravelTimeForPath(gedict_t *m, int i)
 		player_speed = sv_maxspeed * (1 + max(0, DotProduct(distance, hor_distance)));

 		// FIXME: RJ time is guideline, but we can do better than this?
-		m->fb.paths[i].time = 100000;
+		m->fb.paths[i].time = 1000000;


Accidental change? If not it would be great with an explaination.

dsvensson · 2024-09-18T18:04:24Z

src/route_calc.c

+
+		// TODO hiipe - work out time correctly!
+		m->fb.paths[i].time = /*1000000*/(total_distance / player_speed);
+		//m->fb.paths[i].hook_time = (total_distance / player_speed);


Does it work at all? Needs more work? Otherwise as with other stuff, file a GH issue with context and possibly half baked ideas. Perhaps someone else happens to read it and implement it for you compared to hiding it in the code. Should probably start using labels in GH issues for ctf, bots etc in the issues for discoverability.

dsvensson · 2024-09-18T18:06:41Z

src/route_calc.c

+			zone->next_hook = path->next_marker;
+
+			no_change = false;
+		}


move move down to its if (zone->hook_time ... buddy?

dsvensson · 2024-09-18T18:11:03Z

src/teamplay.c

@@ -713,20 +764,38 @@ static void TeamplayReportTaken(gedict_t *client)
 		}
 		else if ((NEED(needFlags, it_health) || NEED(needFlags, it_armor) || NEED(needFlags, it_rl)
 				|| NEED(needFlags, it_lg) || NEED(needFlags, it_rockets)
-				|| NEED(needFlags, it_cells)) && HAVE_POWERUP(client))
+				|| NEED(needFlags, it_cells)))


brainz hurts, getting near bottom, is there any risk of something not HAVE_POWERUP(client) and not isCTF() && (HAVE_FLAG(client) || HAVE_RUNE(client)) slips through here and produces some weird "need " message?

mushis · 2024-09-19T10:33:17Z

This is almost unrelated to this ticket, and im sorry for that. Yesterday I played 2on2 with bots (deathmatch) and when it comes to Quad (well, powerups I believe), they are very stupid. I mean, they steal the Quad while Im there, they get in the way.
I was happy when I read the above:

Bots will abandon goals if a teammate is already going for them, except artefacts and flags.

And I just wanted to make sure this use case has been fixed - not only for CTF but for other game modes too!
If you prefer, I can open a issue.

tcsabina · 2024-10-12T04:27:38Z

@SpookyScaryDev,

How do you see the comments from @dsvensson ?

SpookyScaryDev · 2024-10-18T11:03:00Z

@tcsabina Yes, thanks for the poke. I will try and get this done over the next week! And thanks @dsvensson for all the amazing help and feedback :)

tcsabina · 2024-12-01T19:48:41Z

@tcsabina Yes, thanks for the poke. I will try and get this done over the next week! And thanks @dsvensson for all the amazing help and feedback :)

Hi @SpookyScaryDev,
How do you see this being picked-up?
I think there is not much left to do, right? ;)

SpookyScaryDev · 2024-12-03T17:39:15Z

@tcsabina Yes, thanks for the poke. I will try and get this done over the next week! And thanks @dsvensson for all the amazing help and feedback :)

Hi @SpookyScaryDev, How do you see this being picked-up? I think there is not much left to do, right? ;)

You're right, there is not much left to do, however I'm increadibly busy with uni work at the moment so I've barely had any time to work on this. Hopefully I will have some time over the holidays to finish this up.

dsvensson · 2024-12-04T11:06:09Z

src/bot_movement.c

@@ -434,6 +434,7 @@ void BotSetCommand(gedict_t *self)
 	vec3_t direction;

 	BotPerformRocketJump(self);
+	BotPerformHook(self);


if (isCTF()) {...} here, and perhaps other places.

dsvensson · 2024-12-04T11:07:52Z

src/marker_load.c

@@ -49,6 +49,11 @@ const char* EncodeMarkerFlags(int marker_flags)
 		*s++ = '6';
 	}

+	if (marker_flags & MARKER_E2M2_DISCHARGE)
+	{
+		*s++ = 'd';


Add d to FROGBOT_MARKER_FLAG_OPTIONS.

dsvensson · 2024-12-04T11:17:07Z

src/teamplay.c

+#define TP_NAME_RUNE1      "{&c0f0res&cfff}"
+#define TP_NAME_RUNE2      "{&cf00str&cfff}"
+#define TP_NAME_RUNE3      "{&cff0hst&cfff}"
+#define TP_NAME_RUNE4      "{&c0ffrgn&cfff}"


Unrelated to bots.

dsvensson · 2024-12-04T11:18:48Z

src/teamplay.c

+		}
+		else if (item->ctf_flag & CTF_RUNE_RGN)
+		{
+			client->tp.took.item = it_rune4;


Guess it overlaps with bots, but could be valuable before bots? How does this relate to //ktx took messages to client/mvd, would be pretty nice to get rid of the nasty hack in demo playback / qtv for tracking runes. The stufftext is //ktx took %d %d %d that takes entity id, timer, and taker. If timer is 0, ezq will not create any timer for it.

Also, why not check for classname item_flag_team1 and item_flag_team2 here?

dsvensson · 2024-12-04T11:27:06Z

src/teamplay.c

+				strlcat(message, CTFItemText(client), sizeof(message));
+				strlcat(message, " ", sizeof(message));
+			}
+			strlcat(message, "need ", sizeof(message));


Everything here is unrelated to bots? Just additional tp messaging. Split to separate PR to trim down this big PR, can be merged on its own.

dsvensson · 2024-12-04T11:32:02Z

include/fb_globals.h

@@ -132,6 +136,9 @@ extern gedict_t *dropper;
 #define BOTPATH_CURLJUMP_HINT		(1 << 23)
 #define BOTPATH_FULL_AIRCONTROL		(1 << 24)
 #define BOTPATH_RJ_IN_PROGRESS		(1 << 25)
+#define HOOK                        (1 << 26)
+#define LOOK_BUTTON                 (1 << 27)	// Indicates path which points to a button to shoot 
+#define FIRE_BUTTON                 (1 << 28)   // Set when a bot should try shooting a button


Perhaps move LOOK_BUTTON and FIRE_BUTTON to 26 resp 27 (or I guess leave the 26 hook gap as it will be filled by this PR later), and open a separate PR for adding these to trim down the PR size given that they provide standalone functionality to bots.

dsvensson · 2024-12-04T11:33:43Z

src/bot_botpath.c

@@ -178,6 +178,84 @@ static qbool LookingAtPlayer(gedict_t *self)
 	return (self->fb.look_object && (self->fb.look_object->ct == ctPlayer));
 }

+void LookAtButton(gedict_t* button, qbool buttonIsDoor)


generic, separate PR

dsvensson · 2024-12-04T11:35:17Z

src/bot_botweap.c

+			desired_weapon = IT_LIGHTNING;
+		}
+
+		self->fb.firing |= CheckNewWeapon(desired_weapon);


why not also IT_AXE?

dsvensson · 2024-12-04T11:55:00Z

Still trivial to rebase on top of master at least.

There are some generic functionality in there that can trim down the size of the PR by being submitted as separate PRs.

Generic teamplay messaging unrelated to bots (with a corresponding PR towards ezq with the TP_Msg_blah_f funcs).
Generic LOOK_BUTTON/FIRE_BUTTON bot functionality that works just as well in DM.
Generic /botcmd gomarker that makes all bots want to walk to nearest marker player is pointing at (currently ktx can only teleport bot to a marker). Maybe this one ought to take an optional marker# argument to get the same feel as the existing goto. It should likely also move to the editor_commands where the goto currently resides. Should it be renamed to distinguish from /botcmd goto <marker>?
Generic warning about linking to dynamically created marker.

Divide and conqueror!

dsvensson · 2024-12-04T13:06:33Z

src/teamplay.c

+	}
+	if (isCTF() && (HAVE_RUNE(client) || HAVE_RUNE(client)))
+	{
+		strlcpy(buffer, CTFItemText(client), sizeof(buffer));


Is it intentional that you overwrite the buffer here and everywhere else to keep the message short (can have powerup message in it)?

dsvensson · 2024-12-04T14:15:03Z

src/runes.c

+	TeamplayEventItemTaken(other, self);
+
+#ifdef BOT_SUPPORT
+	if (bots_enabled() && other->isBot)


A bit confused, why do you check other->isBot, but generally not at other similar situations? If not checking that, this would be equivalent to ItemTaken(self, other).

SpookyScaryDev closed this Sep 17, 2024

SpookyScaryDev force-pushed the ctf-bots branch from 2e0f735 to 2b31159 Compare September 17, 2024 15:03

Added NULL check for look_marker in BestEnemy_apply

787ae73

Added CTF bots

4d552fe

SpookyScaryDev reopened this Sep 18, 2024

dsvensson reviewed Sep 18, 2024

View reviewed changes

dsvensson reviewed Dec 4, 2024

View reviewed changes

src/bot_botweap.c

desired_weapon = IT_LIGHTNING;

}

self->fb.firing |= CheckNewWeapon(desired_weapon);

Copy link

Collaborator

dsvensson Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not also IT_AXE?

dsvensson reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ctf Frogbots #365

Ctf Frogbots #365

SpookyScaryDev commented Sep 5, 2024

dsvensson commented Sep 5, 2024 •

edited

Loading

dsvensson commented Sep 17, 2024

dsvensson left a comment •

edited

Loading

dsvensson Sep 18, 2024

dsvensson Sep 18, 2024 •

edited

Loading

dsvensson Sep 18, 2024 •

edited

Loading

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson Sep 18, 2024

dsvensson Sep 18, 2024

dsvensson Sep 18, 2024

dsvensson Sep 18, 2024

dsvensson Sep 18, 2024 •

edited

Loading

dsvensson Sep 18, 2024

dsvensson Sep 18, 2024

mushis commented Sep 19, 2024

tcsabina commented Oct 12, 2024

SpookyScaryDev commented Oct 18, 2024

tcsabina commented Dec 1, 2024

SpookyScaryDev commented Dec 3, 2024

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024 •

edited

Loading

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson Dec 4, 2024

dsvensson commented Dec 4, 2024 •

edited

Loading

dsvensson Dec 4, 2024 •

edited

Loading

dsvensson Dec 4, 2024

Ctf Frogbots #365

Are you sure you want to change the base?

Ctf Frogbots #365

Conversation

SpookyScaryDev commented Sep 5, 2024

General Features / Changes

CTF Support

Known Issues

dsvensson commented Sep 5, 2024 • edited Loading

dsvensson commented Sep 17, 2024

dsvensson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsvensson Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

dsvensson Sep 18, 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

dsvensson Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mushis commented Sep 19, 2024

tcsabina commented Oct 12, 2024

SpookyScaryDev commented Oct 18, 2024

tcsabina commented Dec 1, 2024

SpookyScaryDev commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsvensson Dec 4, 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

dsvensson commented Dec 4, 2024 • edited Loading

dsvensson Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsvensson commented Sep 5, 2024 •

edited

Loading

dsvensson left a comment •

edited

Loading

dsvensson Sep 18, 2024 •

edited

Loading

dsvensson Sep 18, 2024 •

edited

Loading

dsvensson Sep 18, 2024 •

edited

Loading

dsvensson Dec 4, 2024 •

edited

Loading

dsvensson commented Dec 4, 2024 •

edited

Loading

dsvensson Dec 4, 2024 •

edited

Loading