Custom splits #397

fireblizzard · 2023-01-10T22:17:19Z

Why?

I implemented BXT splits with the main goal of being able to use custom splits with Livesplit, that can rely on different entities being activated and not only on a volume that you can go through (like custom triggers or trigger_* entities). There are a few cases where we would need the more fine grained pace information that these custom splits can provide, and where map splits are just too verbose or even useless. There are some key moments in the run where runners analyze their pace and compare to other runs, but don't keep track of it through Livesplit because there's no autosplit there, and it would be very convenient that some events during the run could be used for autosplitting. Some examples of this are:

the button press in the test chamber in Anomalous Materials
the valve activation in the Unforeseen Consequences sewers
the elevator button press to go up to Blast Pit's main area
the fire button press in Blast Pit
the moment you get captured in Apprehension
the button press at tripmine hell

And there are probably a few more that are common pace-checking moments among most runners, and that are not being tracked at least automatically (afaik no HL runner does manual splits or a mix of both, they mostly rely on map/chapter autosplitting). That is for the full run of HL. There are probably a lot more instances where you would want to autosplit in other mods.

Another important case where custom splits would be useful is on Individual Level runs, where runs are much more optimized than a full game run and having more fine grained information is essential. It's applicable to any HL IL nowadays, but the easiest example is Hazard Course. It has lots of stages or places where you would really want to keep track of your pace, because you're trying to save less than a couple seconds in a 2:30 minutes run and losing 400ms in the first couple stages (25s) would already mean a reset. It's inconvenient to wait until the next changelevel to know your pace (you may not remember the pace you had in your PB in all of these stages), because that's 1/3 into the run. Then some changelevels are placed in a way that don't really tell much about your pace in such optimized runs, because you may have entered the trigger with a good time, but bad speed, position and/or angles, due to being inconveniently placed in a corridor or something. It gives inconsistent or useless info about your pace, so in these cases you wish you could replace that split with another one that is more meaningful, harder to trick like a button press or some event that is followed by a cutscene or waiting time, where your speed/position when splitting doesn't really matter or it's gonna get reset anyways.

Lastly, it can be used for practicing microoptimizations in segments inside a map. So basically to break down a map into smaller parts and be able to track the pace in every segment. Thinking not only about campaign maps, but any movement map like the ones played in HLKZ. I have used these custom splits with one HLKZ map and it was pretty useful to optimize the run and use this information later to do actual runs on AG. I'm not familiar with TASes or segmenting, but maybe they can also use custom splits this way, since it's not only meant to work with Livesplit, but it can also tell you the pace ingame through the HUD or console.

So that's the purpose of bxt splits, but why create a whole set of new commands and not work with existing custom triggers and bxt_fire_on_* to make them able to split? I thought about this but discarded the idea mainly for a couple reasons:

BXT triggers are not allowed to be used during a run, and I have no idea how feasible it is to allow some specific usage of them, like splitting. I don't know the tool(s) that mods use to verify runs, i don't know the process. But to be honest i kind of have the same concern for splits. The idea is that they are allowed and can be used for runs, without making them invalid. There could be some information advantage (e.g.: you can know for sure if you pressed or missed a button behind a wall without needing to hear it, if you have a split on it) in the same fashion as having the viewangles in the HUD, but that's allowed so i'm guessing this would be too. There are a couple commands that would have to be disallowed for runs, like the ones to autostart the timer on the first BXT split and autoend on the last one, since the corresponding bxt_timer_ commands are also disallowed IIRC.
There would be too many things specific to splits, to be added for custom triggers, and most of them may not make sense for custom triggers. However, there are some things that splits have and custom triggers could use, like being able to limit them to a certain map. We can add that for custom triggers in another PR.

Pending to resolve

There are some things that can be improved and other things that are not directly related to BXT, but are problems that arise because of it. I'm not sure if i want to resolve them in this PR or if it is good enough to start going. I have been reviewing most if not all of the functionality with Muty, and he thinks this is good to go and can be useful immediately after the next BXT release, and Kananga would like the Livesplit issue (mentioned below) to be resolved to start using custom splits, but that's unrelated to this PR and i'll try to resolve it soon;

There are very likely entities and other events that runners would want to be able to split on, like maybe an NPC dying or something, that are not tracked yet within BXT. I guess we'll be adding patterns and offsets to cover those situations as we need them, gradually.
Custom splits make more sense when used with subsplits on LiveSplit, instead of normal splits. The problem with subsplits, and this is why most runners don't use the map subsplits and instead use full chapter splits, is that LiveSplit doesn't keep track of golds for the split (chapter) itself, only keeps track of gold segments for the subsplits. So if you get the fastest ever AM (Anomalous Materials), it won't show that as gold, AFAIK; but it will show a gold for c1a0 (subsplit in the AM split) if you get the fastest ever c1a0.
I believe ButtonUse would have to check if there's a master entity and if it's triggered, and also check the toggle state because it may be returning to the initial state instead of getting activated. These would conditionate if the button has to get activated or not, and right now we are not taking those into account. I don't think it's necessary for this PR, because it's not a big problem as it is now, at least it hasn't been in my testing, setting up splits on a lot of entities along the HL run. Oh yeah, another problem with this is that as a result, we may be splitting on multi_managers that would be called after the button finishes moving or after the activation delay is completed, which is not exactly what runners may want, since they probably want to track the time they press the button at, and that time +2s or whatever is the delay for that button. Muty says it doesn't really matter, but if we release this as is and then improve this behaviour on a next release, then they may easily gold the split corresponding to the entity with improved behaviour, and they would have a now fake gold in the split that follows this one.
The split times are output to console when the run ends (timer stops) are not sorted by arrival order. They show in the same order as they were added. It's not a big problem because you can just add them in the order you want them to appear, but if there's a map with different routes and you have 2 splits that can be hit in any order depending on the route you choose that run, then it would be pretty useful to have them sorted by arrival.

Ideas for future PRs related to the current one

I should probably create an issue for each these:

Add a enabled/disabled flag for custom triggers (and in consequence, for splits, since it's a derived class). May be useful when practicing, when you are playing around creating triggers and you want to quickly get rid of a trigger but you are not sure if you want to add it back later.
Add a splits history, that keeps track of all the split times/speeds/etc. during the current game session or even stores them in a file. So that you can throw this data on a spreadsheet or whatever, or just to add the capability of comparing against your best time/speed for a split.
Make it so that 2 splits with the same name act as a single one. Use cases: map with different routes.
Add map scope to custom triggers. This would fix the issue of encountering a trigger that you placed for a previous map.
Add stats for splits and for the whole run. Like: average speed, max speed, sync %, distance traveled, jump count, ducktap count, slowdowns count, time on ground, distance on ground.

Should i add an explanation for the command/cvars, or is it enough with the code? you may want it for the wiki too, but i guess anything could change after the code review, so maybe it's better to document that later

Also i implemented most of this back in September, but i got very sick for a month and couldn't really work on anything, and then i got busy with other things, so i postponed this until now. Hopefully that's not a big problem when trying to answer questions about why i did this or that, but i may have forgotten the reasoning behind

About the code style, i just tried to imitate whatever was surrounding the new changes, and in the case of the Split class i tried to imitate the Trigger class. All of this results in having different code styles along the commit

fireblizzard · 2023-01-10T22:36:53Z

I'm not sure how to fix the unresolved external symbols. I don't have this issue in my environment on Windows

khanghugo · 2023-01-11T13:54:40Z

I'm not sure how to fix the unresolved external symbols. I don't have this issue in my environment on Windows

This builds fine for me on Fedora 37. Probably weird stuff with MSVC.

In file included from /home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.cpp:5:
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.hpp: In constructor ‘CustomTriggers::Trigger::Trigger(Vector, Vector)’:
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.hpp:19:36: warning: ‘CustomTriggers::Trigger::corner_max’ will be initialized after [-Wreorder]
   19 |                 Vector corner_min, corner_max;
      |                                    ^~~~~~~~~~
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.hpp:9:29: warning:   ‘std::string CustomTriggers::Trigger::command’ [-Wreorder]
    9 |                 std::string command;
      |                             ^~~~~~~
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.cpp:38:9: warning:   when initialized here [-Wreorder]
   38 |         Trigger::Trigger(Vector corner1, Vector corner2)
      |         ^~~~~~~
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.hpp: In constructor ‘CustomTriggers::Trigger::Trigger(Vector, Vector, std::string)’:
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.hpp:19:36: warning: ‘CustomTriggers::Trigger::corner_max’ will be initialized after [-Wreorder]
   19 |                 Vector corner_min, corner_max;
      |                                    ^~~~~~~~~~
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.hpp:9:29: warning:   ‘std::string CustomTriggers::Trigger::command’ [-Wreorder]
    9 |                 std::string command;
      |                             ^~~~~~~
/home/khang/bxt/pr/fireblizzard/BunnymodXT/BunnymodXT/custom_triggers.cpp:47:9: warning:   when initialized here [-Wreorder]
   47 |         Trigger::Trigger(Vector corner1, Vector corner2, std::string command)
      |         ^~~~~~~

YaLTeR

Thanks for the detailed write-up! Yeah, I can see the appeal here, and it's great to see an implementation.

There are very likely entities and other events that runners would want to be able to split on, like maybe an NPC dying or something, that are not tracked yet within BXT. I guess we'll be adding patterns and offsets to cover those situations as we need them, gradually.

Yeah, that sounds like a good plan to me.

Custom splits make more sense when used with subsplits on LiveSplit, instead of normal splits.

I guess this isn't really a question about the functionality in this PR, but rather how to best use it with LiveSplit.

I believe ButtonUse would have to check if there's a master entity and if it's triggered, and also check the toggle state because it may be returning to the initial state instead of getting activated. These would conditionate if the button has to get activated or not, and right now we are not taking those into account. I don't think it's necessary for this PR, because it's not a big problem as it is now, at least it hasn't been in my testing, setting up splits on a lot of entities along the HL run. Oh yeah, another problem with this is that as a result, we may be splitting on multi_managers that would be called after the button finishes moving or after the activation delay is completed, which is not exactly what runners may want, since they probably want to track the time they press the button at, and that time +2s or whatever is the delay for that button. Muty says it doesn't really matter, but if we release this as is and then improve this behaviour on a next release, then they may easily gold the split corresponding to the entity with improved behaviour, and they would have a now fake gold in the split that follows this one.

Sure, if you and Muty tested it extensively and it works fine, then it's good. As for fake golds, hopefully that kind of "breaking" fix won't be a frequent occurrence and not a big bother to fix the splits manually on the runner's side.

The split times are output to console when the run ends (timer stops) are not sorted by arrival order. They show in the same order as they were added. It's not a big problem because you can just add them in the order you want them to appear, but if there's a map with different routes and you have 2 splits that can be hit in any order depending on the route you choose that run, then it would be pretty useful to have them sorted by arrival.

Yeah, doesn't sound like a blocker for this PR. Can be added later.

Add a splits history, that keeps track of all the split times/speeds/etc. during the current game session or even stores them in a file. So that you can throw this data on a spreadsheet or whatever, or just to add the capability of comparing against your best time/speed for a split.

Isn't this sort of stuff better delegated to LiveSplit or other timers? Not sure how much value there is to reimplementing parts of LiveSplit in BXT.

Should i add an explanation for the command/cvars, or is it enough with the code? you may want it for the wiki too, but i guess anything could change after the code review, so maybe it's better to document that later

I would appreciate if you added the documentation to the wiki page after the PR is merged. I think anyone can edit the wiki page?

Sorry about the CI, Microsoft's servers are acting up for the past few days for some reason (I'm sick of it too). But I see a bunch of new warnings, could you fix them please?

YaLTeR · 2023-01-13T06:59:09Z

BunnymodXT/modules/HwDLL.cpp

+void HwDLL::TimerStart()
+{
+	if (!CustomHud::GetCountingTime())
+	HwDLL::GetInstance().Called_Timer = true;


Suggested change

HwDLL::GetInstance().Called_Timer = true;

HwDLL::GetInstance().Called_Timer = true;

YaLTeR · 2023-01-13T07:01:28Z

BunnymodXT/modules/HwDLL.cpp

+	static void handler()
+	{
+		if (Splits::splits.empty()) {
+			HwDLL::GetInstance().ORIG_Con_Printf("You haven't placed any split triggers.\n");


Suggested change

HwDLL::GetInstance().ORIG_Con_Printf("You haven't placed any split triggers.\n");

HwDLL::GetInstance().ORIG_Con_Printf("You haven't added any splits.\n");

Since there aren't only triggers?

YaLTeR · 2023-01-13T07:02:13Z

BunnymodXT/modules/HwDLL.cpp

+
+struct HwDLL::Cmd_BXT_Split_On
+{
+	USAGE("Usage: bxt_split_on <target_name> [map_name]\n Tells BunnySplit to split when activating an entity by the specified name, and prints to console the current time.\n");


I think I'd like more descriptive names than "bxt_split_on" and "bxt_split_add". How about "bxt_split_on_entity" and "bxt_split_add_trigger"

What about:

bxt_split_on -> bxt_splits_add_entity

bxt_splits_add -> bxt_splits_add_trigger (as you suggested, but in plural)

May be easier to remember and to navigate when using down arrow on the autocomplete dropdown

That works yeah

YaLTeR · 2023-01-13T07:02:43Z

BunnymodXT/modules/HwDLL.cpp

+	static void handler(const char* id_or_name)
+	{
+		// First try to find it by name, otherwise we'll try to find by id
+		const auto itr = std::find_if(Splits::splits.


Weird line break

YaLTeR · 2023-01-13T07:03:16Z

BunnymodXT/modules/HwDLL.cpp

+		// First try to find it by name, otherwise we'll try to find by id
+		const auto itr = std::find_if(Splits::splits.
+			begin(), Splits::splits.end(),
+			[&id_or_name](const Splits::Split& s) { return std::string(id_or_name) == s.get_name(); });


This allocates a new std::string every iteration. It should either do it once before the search or use something like strcmp

YaLTeR · 2023-01-13T07:19:46Z

BunnymodXT/modules/ServerDLL.cpp

}
+

Suggested change

BunnymodXT/modules/ServerDLL.cpp

YaLTeR · 2023-01-13T07:21:30Z

BunnymodXT/modules/ServerDLL.cpp

+			// already have the button's master entity triggered and the button should already be enabled, so it's
+			// better than the ButtonUse in that regard, but it doesn't get called for every button...
+
+			// TODO: check for this button's availability, it may not be usable yet, like the button in the test chamber,


Doesn't the comment above say that the button in this method is in fact usable already?

Yeah the bottom comment is deprecated :/

YaLTeR · 2023-01-13T07:22:04Z

BunnymodXT/patterns.hpp

+
+		PATTERNS(CBaseDoor__DoorActivate,
+			"HL-WON",
+			"51 56 8B F1 57 8D BE ?? ?? ?? ??"


Trailing ?? do not affect the matching

Suggested change

"51 56 8B F1 57 8D BE ?? ?? ?? ??"

"51 56 8B F1 57 8D BE"

Oh yeah, I just blindly copied from makesig, I should have been more careful

YaLTeR · 2023-01-13T07:23:12Z

BunnymodXT/splits.cpp

+		// because this is just a more specific kind of trigger
+	}
+
+	const std::string& Split::get_name() const


Is there any particular reason to add getters and setters for these rather than just working with fields directly? They all seem to just do what they say with no extra logic. The two ones in custom_triggers do have extra logic.

Not really, my brain just had a java moment :)

YaLTeR

Why is the PR marked as draft btw? Do you plan on giving it more testing before merging? I'm asking because I need to release a new BXT version very soon (ppl are waiting for nvidia crash fix), so if this PR needs more testing I won't wait for it.

YaLTeR · 2023-01-14T06:39:50Z

BunnymodXT/modules/HwDLL.cpp


-		Splits::splits.erase(Splits::splits.begin() + (idx - 1));
+		const auto it = std::find_if(Splits::splits.begin(), Splits::splits.end(), [&split](const Splits::Split& s) {


Hm, this won't actually work if this split has an empty name, it's being deleted by id, and there's another split with an empty name before. Or if there are multiple splits with the same name. Maybe worth retaining the search as is then...

True, i have reverted it

fireblizzard · 2023-01-14T08:23:19Z

Why is the PR marked as draft btw? Do you plan on giving it more testing before merging? I'm asking because I need to release a new BXT version very soon (ppl are waiting for nvidia crash fix), so if this PR needs more testing I won't wait for it.

I marked it as a draft initially because i didn't know if you were gonna ask me to fix any of the points in the "Pending to resolve" part, or if you were gonna ask for some big change, so i thought there could still be some discussion to do.

As i mentioned in the writeup, we think this is good enough to go, and we already gave it some testing in HL1 and HC. There could totally be cases that don't work outside these 2, but i believe the current functionality gives it enough flexibility to work around any issues that may arise, and in any case it should be better to go with the current implementation than delaying it further. I tested with the requested changes yesterday for a few hours, and it looks good.

fireblizzard · 2023-01-14T21:11:26Z

I'm not planning on adding anything else for this PR. If you have any more changes to request i'll work on them tomorrow

YaLTeR · 2023-01-14T22:15:17Z

Looks good, thanks a lot!

fireblizzard added 2 commits January 10, 2023 21:44

Implement splits

bf74d9a

Allow splitting on entities by name or target

86020cd

Included splits files to CMakeLists.txt

c9c44ec

YaLTeR requested changes Jan 13, 2023

View reviewed changes

fireblizzard added 12 commits January 13, 2023 15:15

Fix inconsistent mentions to splits

561e371

Fix inconsistent/unnecessary whitespace

dde0167

Remove trailing wildcards

e4ba283

Return earlier when failed to split

0b1d397

Avoid printing splits at the end multiple times

443fe40

Fix export for split entities

5cd8071

Avoid allocating extra string on splits related loops

85ec8b5

Avoid overcomplicated index calculation

3f838f9

Remove wrong comment

9fd0e8f

Remove repeated code by reusing split name/id getter

fe3d3c3

Initialize members in the same order as declared

b6e03b1

Remove getters/setters in favor of direct field usage

c9fc749

YaLTeR requested changes Jan 14, 2023

View reviewed changes

fireblizzard added 2 commits January 14, 2023 10:06

Rename split creation functions

6ba9d15

Revert bxt_splits_delete to previous search&erase

c52c31c

fireblizzard marked this pull request as ready for review January 14, 2023 09:23

Limit split that is being created to current map

dea420b

YaLTeR merged commit 869253a into YaLTeR:master Jan 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom splits #397

Custom splits #397

fireblizzard commented Jan 10, 2023 •

edited

Loading

fireblizzard commented Jan 10, 2023

khanghugo commented Jan 11, 2023 •

edited

Loading

YaLTeR left a comment

YaLTeR Jan 13, 2023

YaLTeR Jan 13, 2023

YaLTeR Jan 13, 2023

fireblizzard Jan 13, 2023

YaLTeR Jan 14, 2023

YaLTeR Jan 13, 2023

YaLTeR Jan 13, 2023

YaLTeR Jan 13, 2023

YaLTeR Jan 13, 2023

fireblizzard Jan 13, 2023

YaLTeR Jan 13, 2023

fireblizzard Jan 13, 2023

YaLTeR Jan 13, 2023

fireblizzard Jan 13, 2023

YaLTeR left a comment

YaLTeR Jan 14, 2023

fireblizzard Jan 14, 2023

fireblizzard commented Jan 14, 2023

fireblizzard commented Jan 14, 2023

YaLTeR commented Jan 14, 2023

	HwDLL::GetInstance().Called_Timer = true;
	HwDLL::GetInstance().Called_Timer = true;

	HwDLL::GetInstance().ORIG_Con_Printf("You haven't placed any split triggers.\n");
	HwDLL::GetInstance().ORIG_Con_Printf("You haven't added any splits.\n");


		Splits::splits.erase(Splits::splits.begin() + (idx - 1));
		const auto it = std::find_if(Splits::splits.begin(), Splits::splits.end(), [&split](const Splits::Split& s) {

Custom splits #397

Custom splits #397

Conversation

fireblizzard commented Jan 10, 2023 • edited Loading

Why?

Pending to resolve

Ideas for future PRs related to the current one

fireblizzard commented Jan 10, 2023

khanghugo commented Jan 11, 2023 • edited Loading

YaLTeR left a 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

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

YaLTeR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fireblizzard commented Jan 14, 2023

fireblizzard commented Jan 14, 2023

YaLTeR commented Jan 14, 2023

fireblizzard commented Jan 10, 2023 •

edited

Loading

khanghugo commented Jan 11, 2023 •

edited

Loading