feat(enhancement): Add "timer" trigger for missions #9089

danaris · 2023-07-28T20:57:58Z

Feature: This PR implements the feature request detailed and discussed in issue #4475

Adds a new timer element to mission specifications, allowing for actions to trigger after a certain amount of time spent in a given system.

Feature Details

The element has the following format:

timer "<Timer Name>" <base time> [random time]
system "<system name>"
[idle]
[uncloaked]
[proximity <radius> [close|far]]
["<planet name>"]
on timeup
<action/conversation>

UI Screenshots

N/A

Usage Examples

This PR includes a modification of the "Sad Archie" mission to use the timer rather than the existing Timer Ship mechanism.

Testing Done

Automated Tests Added

I have no experience with creating tests, and am unclear on whether this needs them.

Performance Impact

Performance impact should be minimal. It adds a loop through active missions to check for timers, and through the active timers to step through them, but each call of the Timer::Step() method is quite lightweight, and no appreciable performance impact was observed.

danaris · 2023-08-01T03:04:27Z

As I was playing through the Wanderer campaign, I noticed another place where it seemed like the timer functionality would be very useful: in the "Wanderers: Kor Mereti Hacking" mission.

I changed the on enter Mekislepti trigger to the following timers:

	timer "Kor Mereti Hacking Timer A" 5
		system Mekislepti
		uncloaked
		reset "leave system"
		on timeup
			"reputation: Kor Mereti" = 1
	
	timer "Kor Mereti Hacking Timer B" 10
		system Mekislepti
		uncloaked
		on timeup
			"reputation: Kor Mereti" = -10
			dialog
				`When you enter the Mekislepti system, an entire fleet of drones moves to intercept you. Hardly daring to hope, you press the button that activates the Wanderer program. For a fraction of a second, the drones' engines stop firing and they drift as if out of control. Then, with renewed purpose, they move to attack you.`
				`	Oh well. It was worth a shot.`

...and the result produced this video.

It does make me think we might need another flag for timers: one that prevents them from stopping or resetting for any reason. Without something like that, there would be a danger, in this case, of the player leaving the system within the 5 seconds that the Kor Mereti are friendly; I don't know if that would be useful, but it is at least a concern.

quyykk

Nice!

Timers basically have a condition that they must fulfill to run (in system, in zone, idling, etc.). If they stop fulfilling this, the timer should by default stop and reset IMO. I think that's a good default to have, because that's usually what you want.

source/Mission.h

source/Timer.cpp

source/Timer.h

source/Timer.cpp

data/sheragi/archaeology missions.txt

quyykk · 2023-08-31T10:31:29Z

source/Timer.h

+	void Step(PlayerInfo &player, UI *ui);
+
+private:
+	enum ResetCondition { NONE, PAUSE, LEAVE_ZONE, LEAVE_SYSTEM };


Enum class please

Again, my lack of C++ experience shows: is it as simple as adding class after the enum? Or is more required here?

Yeah basically. You don't need to make the values all caps either

...Is there a Style Guide on that last part? Because most, though not all, such enums do currently seem to have all-caps names...

source/PlayerInfo.cpp

source/Timer.cpp

source/MainPanel.cpp

source/Timer.cpp

source/Timer.h

source/Timer.cpp

quyykk · 2023-09-16T20:59:47Z

source/Timer.cpp

+				if(proximityObject.HasValidPlanet())
+					centers.push_back(proximityObject.Position());
+		}
+		for(Point &center : centers)


this can be const&. Also instead of adding all the centers to a list, you can check immediately if that specific center is in proximity.

Ah, good call.

If I do that, is there still anything here that should be made const &?

danaris · 2023-09-19T00:26:53Z

It occurred to me that it might also be useful, at least sometimes, to have the ability to fire an action when a timer resets (to let the player know that they're no longer on target or whatever), so I've added that.

source/Mission.cpp

source/Timer.cpp

… check

…ion name, for passing to the action

Co-authored-by: Nick <quyykk@protonmail.com>

- Change the time parameter from seconds to frames - Change the default reset behavior from "none" to "pause" - Add support for a location filter, rather than only a single system specification

Co-authored-by: Nick <quyykk@protonmail.com>

- Change "systems" token to "system" - Make sure that checking proximity works properly even with multiple instances of specified planet (eg, ringworlds) - Make mission argument to Load & constructor const

danaris · 2024-01-10T20:42:00Z

I've added some comments to Timer.cpp. Please let me know if they seem helpful or if they still need work.

RisingLeaf · 2024-01-10T20:49:04Z

I've added some comments to Timer.cpp. Please let me know if they seem helpful or if they still need work.

Better 👍

… to step them into a new Mission method, to avoid having mutual includes between PlayerInfo and Timer

…pecified as proximity objects in the Timer at instantiation, so this doesn't have to happen every Step()

source/Timer.h

source/Mission.cpp

Co-authored-by: Rising Leaf <85687254+RisingLeaf@users.noreply.github.com>

…std::set at RisingLeaf's suggestion

tests/integration/config/plugins/integration-tests/data/tests/tests_timers.txt

RisingLeaf · 2024-01-12T13:09:18Z

source/Mission.h

@@ -160,6 +161,10 @@ class Mission {
 	// Get a list of NPCs associated with this mission. Every time the player
 	// takes off from a planet, they should be added to the active ships.
 	const std::list<NPC> &NPCs() const;
+	// Get a list of timers associated with this mission.
+	std::list<Timer> &Timers();


again I dont think we need a list here, a std set would be enough?
also we dont need this getter anymore right?

You're right; the getter is no longer being called anywhere

Again, I'm afraid I'm not well-acquainted with the distinctions between various STL collection types—I simply copied the usage for NPCs. Is it that there's something different about how Timers work than NPCs that means they should have a set while NPCs have a list, or would this also apply to the Mission's NPC collection?

And am I correct in my understanding that the (primary?) difference between the two is that list can have duplicate members, while set cannot?

And am I correct in my understanding that the (primary?) difference between the two is that list can have duplicate members, while set cannot?
yes

Again, I'm afraid I'm not well-acquainted with the distinctions between various STL collection types—I simply copied the usage for NPCs. Is it that there's something different about how Timers work than NPCs that means they should have a set while NPCs have a list, or would this also apply to the Mission's NPC collection?

dont know

Also remove a line from the tests that seems obsolete

tests/integration/config/plugins/integration-tests/data/tests/tests_timers.txt

source/Mission.h

source/Timer.h

source/Timer.cpp

Amazinite · 2024-01-20T22:53:31Z

source/Timer.cpp

+	bool reset = cond == resetCondition;
+	reset |= (cond == Timer::ResetCondition::LEAVE_ZONE && resetCondition == Timer::ResetCondition::PAUSE);
+	reset |= (cond == Timer::ResetCondition::LEAVE_SYSTEM && (resetCondition == Timer::ResetCondition::PAUSE
+				|| resetCondition == Timer::ResetCondition::LEAVE_ZONE));


This seem difficult to read to me. So we reset if the input condition matches the reset condition for this timer, but the input condition and reset condition can differ and we still reset the timer? I feel like calling ResetOn should just always reset the timer, and we only call the function when we need to, instead of calling the function at every possible chance and having the function figure out if it should actually reset.

I believe I moved this in here to avoid duplication, as it would need to be checked every time a reset might happen.

I've added some comments to clarify the intention here, which I have pasted below:

// If the reset condition being passed in is LEAVE_ZONE (ie, the player is no longer in proximity), // then we also reset if the timer's reset condition is PAUSE (ie, player no longer meets the criteria). ... // If the reset condition being passed in is LEAVE_SYSTEM, then we also reset if either of the other two // conditions is specified for the timer, since we are then necessarily outside the zone, and thus the // clock is stopped.

Please let me know if you still think these tests should be broken back out to the call sites.

source/Timer.cpp

Co-authored-by: Amazinite <jsteck2000@gmail.com>

- Save and reload the elapsed time - Fix some style issues - Add some comments to a confusing code section

danaris · 2024-05-17T14:44:31Z

I believe I've gotten all the comments that were not full sentences. The only conversations still open on this are ones where I'm waiting on a reply—and I believe they're also very minor/matters of opinion.

I'd really love to see if this can make it into 0.10.7 next week!

TomGoodIdea · 2024-06-10T23:22:35Z

source/Timer.cpp

+		else if(child.Token(0) == "system" && child.Size() > 1)
+			system = GameData::Systems().Get(child.Token(1));
+		else if(child.Token(0) == "system")
+			systems.Load(child);
+		else if(child.Token(0) == "proximity" && child.Size() > 1)
+			proximityCenter = GameData::Planets().Get(child.Token(1));
+		else if(child.Token(0) == "proximity")
+			proximityCenters.Load(child);


save some checks

Suggested change

else if(child.Token(0) == "system" && child.Size() > 1)

system = GameData::Systems().Get(child.Token(1));

else if(child.Token(0) == "system")

systems.Load(child);

else if(child.Token(0) == "proximity" && child.Size() > 1)

proximityCenter = GameData::Planets().Get(child.Token(1));

else if(child.Token(0) == "proximity")

proximityCenters.Load(child);

else if(child.Token(0) == "system")

{

if(child.Size() > 1)

system = GameData::Systems().Get(child.Token(1));

else

systems.Load(child);

}

else if(child.Token(0) == "proximity")

{

if(child.Size() > 1)

proximityCenter = GameData::Planets().Get(child.Token(1));

else

proximityCenters.Load(child);

}

TomGoodIdea · 2024-06-10T23:23:53Z

source/Timer.cpp

+	if(system && (proximityCenter || !proximityCenters.IsEmpty()))
+		for(const StellarObject &proximityObject : system->Objects())
+			if(proximityObject.HasValidPlanet() && (proximityCenter == proximityObject.GetPlanet() ||
+				proximityCenters.Matches(proximityObject.GetPlanet())))


Suggested change

proximityCenters.Matches(proximityObject.GetPlanet())))

proximityCenters.Matches(proximityObject.GetPlanet())))

looks clearer

TomGoodIdea · 2024-06-10T23:24:24Z

source/Timer.cpp

+		bool shipIdle = true;
+		if(requireIdle)
+			shipIdle = (!flagship->IsThrusting() && !flagship->IsSteering()
+						&& !flagship->IsReversing() && flagship->Velocity().LengthSquared() < idleMaxSpeed);


Suggested change

&& !flagship->IsReversing() && flagship->Velocity().LengthSquared() < idleMaxSpeed);

&& !flagship->IsReversing() && flagship->Velocity().LengthSquared() < idleMaxSpeed);

TomGoodIdea · 2024-06-10T23:29:33Z

source/Timer.cpp

+		if(proximityCache.size() > 0)
+			for(const StellarObject *proximityObject : proximityCache)
+			{
+				double dist = flagship->Position().Distance(proximityObject->Position());
+				if((closeTo && dist <= proximity) || (!closeTo && dist >= proximity))
+				{
+					inProximity = true;
+					break;
+				}
+			}


if proximityCache is empty, the loop doesn't do anything anyway; I'd also use a ternary for the proximity conditions

Suggested change

if(proximityCache.size() > 0)

for(const StellarObject *proximityObject : proximityCache)

{

double dist = flagship->Position().Distance(proximityObject->Position());

if((closeTo && dist <= proximity) || (!closeTo && dist >= proximity))

{

inProximity = true;

break;

}

}

for(const StellarObject *proximityObject : proximityCache)

{

double dist = flagship->Position().Distance(proximityObject->Position());

if(closeTo ? dist <= proximity : dist >= proximity)

{

inProximity = true;

break;

}

}

If you really want to simplify the conditional just do closeto != (dist >= proximity)
Even though that would ignore the single case of == but that's not important for double values

Hecter94 added the mechanics Things dealing with the mechanics & code of how the game works label Aug 2, 2023

danaris force-pushed the mission-timer branch from db68372 to 0f9ab3d Compare August 28, 2023 19:46

quyykk reviewed Aug 31, 2023

View reviewed changes

Amazinite modified the milestone: 0.10.5 Sep 10, 2023

quyykk reviewed Sep 11, 2023

View reviewed changes

quyykk reviewed Sep 17, 2023

View reviewed changes

quyykk reviewed Sep 21, 2023

View reviewed changes

source/Mission.cpp Outdated Show resolved Hide resolved

source/Timer.cpp Outdated Show resolved Hide resolved

danaris force-pushed the mission-timer branch from 7c63f58 to 02e4738 Compare September 23, 2023 16:13

quyykk reviewed Sep 26, 2023

View reviewed changes

source/Timer.cpp Outdated Show resolved Hide resolved

source/Timer.cpp Outdated Show resolved Hide resolved

source/Timer.cpp Outdated Show resolved Hide resolved

quyykk reviewed Oct 1, 2023

View reviewed changes

source/Timer.cpp Show resolved Hide resolved

source/Timer.cpp Outdated Show resolved Hide resolved

Timothy Collett and others added 17 commits October 7, 2023 14:37

feat(enhancement): Add "timer" trigger for missions

fc3742d

fix(style): Fix issues with whitespace and line wrapping

d167c54

fix(style): one more trailing whitespace

3609b45

hopefully this works

0168279

fix(mechanics): Correctly save the timer even when it has a proximity…

fc8f25b

… check

fix(mechanics): Update to use the actual mission rather than the miss…

6c74a1e

…ion name, for passing to the action

fix(mechanics): Remove requirement for a timer to have a system

7f3ea57

fix(test): See if this makes the new timer test succeed

1ee993c

fix(tests): Increase watchdog further to allow time for test to complete

33cb4a5

fix(mechanics): Apply suggestion from code review

2bd6499

Co-authored-by: Nick <quyykk@protonmail.com>

fix(mechanics): Changes from code review, including:

c061547

- Change the time parameter from seconds to frames - Change the default reset behavior from "none" to "pause" - Add support for a location filter, rather than only a single system specification

fix(mechanics): Fix a mismatched type name that was missed last time

50b4f7a

fix(style): Adjust whitespace & line length

3b669ec

Apply suggestion from code review

ca98196

Co-authored-by: Nick <quyykk@protonmail.com>

fix(mechanics): Suggestions from code review:

e287ed7

- Change "systems" token to "system" - Make sure that checking proximity works properly even with multiple instances of specified planet (eg, ringworlds) - Make mission argument to Load & constructor const

fix(style): Remove extraneous whitespace line

6dd88fe

feat(mechanics): Add an optional "on reset" action to Timers

2ae5a59

docs: comment Timer.cpp per reviewer request

44807c4

Timothy Collett added 3 commits January 11, 2024 14:31

refactor(mechanics): Move the actual iteration through mission timers…

29be8e9

… to step them into a new Mission method, to avoid having mutual includes between PlayerInfo and Timer

refactor(mechanics): Build a cache of StellarObjects matching those s…

d5de638

…pecified as proximity objects in the Timer at instantiation, so this doesn't have to happen every Step()

fix(style): Missed one trailing whitespace

736ef0d

RisingLeaf reviewed Jan 11, 2024

View reviewed changes

source/Timer.h Outdated Show resolved Hide resolved

source/Mission.cpp Outdated Show resolved Hide resolved

danaris and others added 2 commits January 11, 2024 16:42

Apply suggestion from code review

95620f4

Co-authored-by: Rising Leaf <85687254+RisingLeaf@users.noreply.github.com>

refactor(mechanics): change proximity object cache from std::list to …

e65b979

…std::set at RisingLeaf's suggestion

RisingLeaf reviewed Jan 12, 2024

View reviewed changes

tests/integration/config/plugins/integration-tests/data/tests/tests_timers.txt Outdated Show resolved Hide resolved

RisingLeaf reviewed Jan 12, 2024

View reviewed changes

refactor(mechanics): Remove no-longer-necessary accessor

7dcea66

Also remove a line from the tests that seems obsolete

RisingLeaf reviewed Jan 12, 2024

View reviewed changes

tests/integration/config/plugins/integration-tests/data/tests/tests_timers.txt Show resolved Hide resolved

Amazinite reviewed Jan 20, 2024

View reviewed changes

danaris and others added 3 commits January 21, 2024 14:54

Apply suggestions from code review

7383628

Co-authored-by: Amazinite <jsteck2000@gmail.com>

fix(mechanics): Several more code review suggestions from Amazinite:

45f849e

- Save and reload the elapsed time - Fix some style issues - Add some comments to a confusing code section

fix(style): remove extraneous space

5f36e0d

ziproot mentioned this pull request Feb 3, 2024

fix(balance): Fix the timer ship AI #9751

Merged

bene-dictator mentioned this pull request Apr 17, 2024

feat(data): Creating Unclassified and Unclassified Minor Categories #9986

Open

Saugia added waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. waiting on dev A developer needs to do something, e.g. reviewing, merging, resolving disputes, etc. labels May 12, 2024

danaris added 3 commits May 17, 2024 10:39

fix(style): missing period on comment

f572a58

fix(test): Give the timer test a little more time to wait

ee66a27

fix(style): missing period on comment

62e64dc

danaris requested a review from Amazinite May 17, 2024 14:42

ziproot approved these changes May 17, 2024

View reviewed changes

bene-dictator removed the waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. label May 18, 2024

Merge remote-tracking branch 'upstream/master' into pr/9089

0c51ad1

TomGoodIdea reviewed Jun 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(enhancement): Add "timer" trigger for missions #9089

feat(enhancement): Add "timer" trigger for missions #9089

danaris commented Jul 28, 2023 •

edited

Loading

danaris commented Aug 1, 2023 •

edited

Loading

quyykk left a comment

quyykk Aug 31, 2023

danaris Aug 31, 2023

quyykk Aug 31, 2023

danaris Sep 2, 2023

quyykk Sep 16, 2023

danaris Sep 18, 2023

danaris commented Sep 19, 2023

danaris commented Jan 10, 2024

RisingLeaf commented Jan 10, 2024

RisingLeaf Jan 12, 2024

danaris Jan 12, 2024

RisingLeaf Jan 12, 2024

Amazinite Jan 20, 2024

danaris Jan 21, 2024

danaris commented May 17, 2024

TomGoodIdea Jun 10, 2024

TomGoodIdea Jun 10, 2024

TomGoodIdea Jun 10, 2024

TomGoodIdea Jun 10, 2024

RisingLeaf Jun 11, 2024 •

edited

Loading

	proximityCenters.Matches(proximityObject.GetPlanet())))
	proximityCenters.Matches(proximityObject.GetPlanet())))

	&& !flagship->IsReversing() && flagship->Velocity().LengthSquared() < idleMaxSpeed);
	&& !flagship->IsReversing() && flagship->Velocity().LengthSquared() < idleMaxSpeed);

feat(enhancement): Add "timer" trigger for missions #9089

Are you sure you want to change the base?

feat(enhancement): Add "timer" trigger for missions #9089

Conversation

danaris commented Jul 28, 2023 • edited Loading

Feature Details

UI Screenshots

Usage Examples

Testing Done

Automated Tests Added

Performance Impact

danaris commented Aug 1, 2023 • edited Loading

quyykk 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

danaris commented Sep 19, 2023

danaris commented Jan 10, 2024

RisingLeaf commented Jan 10, 2024

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

danaris commented May 17, 2024

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

RisingLeaf Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

danaris commented Jul 28, 2023 •

edited

Loading

danaris commented Aug 1, 2023 •

edited

Loading

RisingLeaf Jun 11, 2024 •

edited

Loading