Allow the creation of bays that can carry ships of multiple types #9906

Amazinite · 2024-03-10T19:12:17Z

Feature

This PR addresses the bug/feature described in issue #9896.

Summary

This PR adds a new root node bay that behaves as follows:

bay <name>: A named bay type. When a ship definition includes a bay point, that bay will be able to store ships from the matching bay root node. If no bay root node matches the name of the ship's bay, then it is assumed that the name of the ship's bay matches a singular category of ship that can be carried. This is done for backwards compatibility.
- This root node takes single token strings as children, similar to the category root node. The children strings are the categories of ship that bays of this type can store. The category must also be present in the category "bay type" root node.
- A named bay is added to whenever it is redefined (such as by a plugin), instead of overwriting it.

This PR only adds the mechanics for bay root nodes. No bay root nodes are added by this PR.

Usage examples

# The categories of ship that can be carried in bays.
category "bay type" 
	"Drone" 
	"Fighter"
	"Bomber"

# Fighter bays carry both fighters and drones.
bay "Fighter"
	"Drone"
	"Fighter"

# Drone bays carry only drones.
bay "Drone"
	"Drone"

ship "Cooler Carrier"
	# This "Fighter" bay refers to the "Fighter" bay node instead of the "Fighter" category,
	# meaning that it can carry both fighters and drones.
	bay "Fighter" x y
	# This "Bomber" bay has no matching "Bomber" bay root node. Therefore, we assume
	# that this bay may only hold the "Bomber" ship category.
	bay "Bomber" x y

Testing Done

Launched the game with no bay root nodes defined.
- Bought a Carrier (6 drone bays, 4 fighter bays), 6 drones, and 4 fighters. Received no flight check warnings for my ships and was able to deploy and recall my fighters.
- Purchased 10 drones and 0 fighters. Received flight check warnings for 4 of the drones. Upon launching, 4 of the drones weren't docked. After deploying and recalling, there were still 4 stray drones.
- Purchased 0 drones and 10 fighters. Received flight check warnings for 6 of the fighters. Upon launching, 6 of the fighters weren't docked. After deploying and recalling, there were still 6 stray fighters.
Launched the game with the "Fighter" and "Drone" bay root nodes from the usage examples.
- Using the same Carrier, purchased 4 fighters and 6 drones, IN THAT ORDER. Received no flight check warnings.
  - When I took off, all my ships were docked. Deploying worked fine. Upon recalling, only the drones would redock. Sometimes a fighter would redock, but as soon as a drone would dock, the fighters wouldn't dock anymore. Seems like the fighter bays were being filled first, so as soon as the drones would dock, the fighters would be unable to do so. Fixed by f7f7558
  - Flipped the order that I purchased the ships so that it was 6 drones then 4 fighters. On take-off, the ships appear to be filling the bays in their order in your ships list, meaning the drones filled the Carrier first and therefore stole the fighter bays from the fighters. Fixed by f7f7558
- Purchased 1 additional fighter for a total of 5 fighters and 6 drones. Received a flight check warning for it, as I had 5 fighters but only space for 4.
- Purchased 1 additional drone for a total of 4 fighters and 7 drones. Received a flight check warning for one of my fighters, as I have space for up to 10 drones total and purchasing the drone kicked a fighter out of its bay.
  - NOTICE: When I took off, there was a stray drone that wasn't docked even though it was a fighter that had the flight check warning. I don't know how much importance we want to put on making it so that the flight checks for extra carried ships exactly match which ships aren't carried when you take off, though.
- Purchased 10 drones. Received no flight check warnings for any of them. Every drone was able to deploy and recall.
- Purchased 10 fighters. Received flight check warnings for 6 of them. Only 4 were able to deploy and redock at a time.

Amazinite · 2024-03-10T19:18:29Z

Opening as a draft to show that this is being worked on. Noting that the snag I caught during testing is a rather large one.

Using the same Carrier, purchased 6 drones and 4 fighters. Received no flight check warnings.

When I took off, all my ships were docked. Deploying worked fine. Upon recalling, only the drones would redock. Sometimes a fighter would redock, but as soon as a drone would dock, the fighters wouldn't dock anymore. Seems like the fighter bays were being filled first, so as soon as the drones would dock, the fighters would be unable to do so.

This is likely happening because the bays are filled in the same order they are defined. On the Carrier, the fighter bays are defined first, and so the drones fill the fighter bays first. The order that the bays are defined in should not matter, though.

I imagine we could make it so that a carrier more intelligently fills its bays by shuffling carried ships to the most restrictive bays first (e.g. if there is a ship of type X docking and the ship has a bay that holds both X and Y and another bay that holds only X, put the ship in the bay that holds only X first), but then we run into issues of having multiple carriers in the same fleet.

Say you have two Carriers. That makes 12 drones and 8 fighters. A Carrier can hold up to 10 drones, or 6 drones and 4 fighters. What happens if you recall and 10 of your drones go to one Carrier and 2 go to the other? Well the second carrier would still have space for 4 fighters, but now you have 4 fighters flying around with nowhere to dock.

Right now when recalling, fighters and drones basically just go to the first ship they can find that can hold them. If we want to be able to have bays that can hold multiple ship categories, we need to basically reserve every bay the moment that the recall occurs in an order that ensures that every carried ship can dock.

Either that, or we completely do away with the fighter/drone distinction for the sake of simplicity. I don't think that's the way we should go if we don't need to, though.

tibetiroka · 2024-03-10T19:22:20Z

Right now when recalling, fighters and drones basically just go to the first ship they can find that can hold them. If we want to be able to have bays that can hold multiple ship categories, we need to basically reserve every bay the moment that the recall occurs in an order that ensures that every carried ship can dock.

Is it not possible to make use of the ships' parent attribute for this, making them board their parent if possible? Or would that break every other use case? (I recall a PR from a couple months/years ago that had something like this, not sure what happened to that.)

On a separate note, how does this handle plugins adding ship categories to existing bay types?

tibetiroka · 2024-03-10T19:24:17Z

An alternative solution would be to have ships take off from a docked state to rearrange themselves, if only a portion of them were docked before. (This would have issues with disabled ships not being able to take off, and would be a bit of a UX nightmare, but might help as a stopgap measure.)

Amazinite · 2024-03-10T19:28:27Z

On a separate note, how does this handle plugins adding ship categories to existing bay types?

A named bay is added to whenever it is redefined (such as by a plugin), instead of overwriting it.

Amazinite · 2024-03-10T19:30:07Z

An alternative solution would be to have ships take off from a docked state to rearrange themselves, if only a portion of them were docked before. (This would have issues with disabled ships not being able to take off, and would be a bit of a UX nightmare, but might help as a stopgap measure.)

We'd need to be able to intelligently reorder carried ships in the first place to even be able to do that, so might as well just dock them in an intelligent order to begin with.

Amazinite · 2024-03-10T19:31:45Z

Is it not possible to make use of the ships' parent attribute for this, making them board their parent if possible? Or would that break every other use case?

That, plus intelligent docking order, would likely solve most of the problem. The only extra case to account for would be intelligently parenting new ships that you capture.

(I recall a PR from a couple months/years ago that had something like this, not sure what happened to that.)

Are you thinking of this one? #6130

tibetiroka · 2024-03-10T19:43:29Z

That, plus intelligent docking order, would likely solve most of the problem. The only extra case to account for would be intelligently parenting new ships that you capture.

It's a bit more than that, since you can lose and gain both carriers and fighters, so it might be necessary to re-dock some ships to be able to fit everything in, even if the order was correct previously. (This is amplified if not all ships were requested to dock initially, or there wasn't enough space the first time the order was given.)

Are you thinking of this one? #6130

Yeah that PR seems to have some useful functionality. Tieing bays and carried ships together would probably allow for caching a bunch of the necessary calculations, but isn't enough when ships are lost (or gained I guess).

Amazinite · 2024-03-10T20:14:30Z

The way this is shaping up, that could be a whole PR of its own changing how fighters and drones reparent.

I'm tempted to say we tackle intelligent docking order here so that given a single carrier everything looks good when taking off and recalling, then any sort of permanent parenting and recalculating parents when fighters are gained or carriers are lost could be done next.

Nothing changes anyway until we add any bay root nodes, so it's not like merging just this would break anything.

xX-Dillinger-Xx · 2024-03-10T20:34:19Z

Just a dumb question, but why not just have 1 'bay type' that allows both drones and/or fighters to use it?

I assume that this might mess with plugin compatibility

Amazinite · 2024-03-10T20:38:48Z

We could very easily have that be the case for vanilla, even without this PR or any other engine updates, but then that leaves no customization options for plugins. Compatiblity would be fine.

What I'm doing here though is adding capabilities to the engine. How vanilla or any plugins make use of the engine's mechanics is a separate topic.

xX-Dillinger-Xx · 2024-03-10T20:50:11Z

Don't mind me I'm just trying learn, I hope I'm not becoming a pest.
I understand your trying to accommodate customizations, I am just offering up a possible option if what your trying do becomes too complicated.

Just keeping it simple, whats happens if:

keep fighter bays < only accepts fighter
keep drone bays < only accepts drones
add new 'universal' bay < accepts anything in a predetermined list

Am I right in assuming all existing ship builds in vanilla and plugins would continue as is. ?
Any edited or new ship builds should only contain 'universal' alone OR a mix of fighter and drone.

I afraid that adding even more calculations may have a negative impact to people with weak GPU, like me.

Amazinite · 2024-03-10T22:14:44Z

I afraid that adding even more calculations may have a negative impact to people with weak GPU, like me.

GPUs are for drawing the frames. Gameplay calculations, which this would be, are for the CPU.

…ble bay

Amazinite · 2024-03-12T08:20:24Z

When a ship docks with a carrier, the carrier now docks the ship in the most restrictive bay first, where restrictiveness is measured by the number of categories the bay allows. If a drone is docking and a carrier has two bays, one that carries only drones and one that carries both drones and fighters, then the first of those two bays is considered the more restrictive and the drone is placed there. That way, drones don't hog fighter bays from fighters when they could've used a drone bay.

source/BayType.cpp

source/Ship.cpp

RisingLeaf · 2024-03-12T11:00:02Z

source/BayType.h

+
+private:
+	std::string name;
+	std::set<std::string> categories;


This is just a side note, but I did some reading on the stl recently and it seems like a std set is actually slower than a std vector in this situation, especially with such a low element count.

To give an example, this code:

#include <set> #include <string> #include <vector> int main() { std::vector<std::string> testE = {"a", "b", "c", "d", "e"}; std::set<std::string> test = {"a", "b", "c", "d"}; for(int x = 0; x < 200000000; x++) int i = test.count(testE[x % 3]); return 0; }

has a worse runtime than this one:

#include <vector> #include <string> #include <algorithm> int main() { std::vector<std::string> testE = {"a", "b", "c", "d", "e"}; std::vector<std::string> test = {"a", "b", "c", "d"}; for(int x = 0; x < 200000000; x++) bool it = std::find(test.begin(), test.end(), testE[x % 3]) == test.end(); return 0; }

This does not really matter, but I just wanted to note this here as I saw a lot of those really useless set use-cases.

Eh. set.count(val) is more readable than find(vector.begin(), vector.end(), val) != vector.end(). Even if the runtime is worse, it doesn't actually result in any tangible difference at the scales we're working at. Checking the categories of a bay isn't exactly a common action, relatively speaking. So this is one of those "We'll change it if it ever actually causes a problem" things.

yea was just giving random thoughts

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

source/BayType.cpp

Koranir

Looks good to me.

source/Ship.cpp

Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com> Co-authored-by: Daniel <101683475+Koranir@users.noreply.github.com>

source/Ship.cpp

source/Ship.h

source/GameData.h

RisingLeaf · 2024-05-11T20:31:52Z

as thee did request, h're is thy revieweth

source/PlayerInfo.cpp

… the namespace

Because of course you should do the true case first Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>

source/Ship.cpp

source/Ship.h

Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>

TomGoodIdea

looks good

source/Ship.cpp

Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>

RisingLeaf · 2024-05-12T12:04:20Z

source/PlayerInfo.cpp

@@ -1101,9 +1104,15 @@ map<const shared_ptr<Ship>, vector<string>> PlayerInfo::FlightCheck() const
 			if(ship->CanBeCarried() || !ship->HasBays())
 				continue;

-			for(auto &bay : ship->Bays())
+			auto &bays = ship->Bays();


Suggested change

auto &bays = ship->Bays();

const auto &bays = ship->Bays();

RisingLeaf · 2024-05-12T12:04:50Z

source/PlayerInfo.cpp

+				if(bay.bayType)
+					for(const string &category : bay.bayType->Categories())
+						++bayCount[category];
+				else


this looks confusing, I would actually prefer the {} here

RisingLeaf · 2024-05-12T12:05:24Z

source/PlayerInfo.cpp

-			for(auto &bay : ship->Bays())
+			auto &bays = ship->Bays();
+			totalBays += bays.size();
+			for(auto &bay : bays)


Suggested change

for(auto &bay : bays)

for(const auto &bay : bays)

RisingLeaf · 2024-05-12T12:06:48Z

source/Ship.cpp

+	if(bayType)
+		return bayType->Contains(category);
+	return name == category;


Suggested change

if(bayType)

return bayType->Contains(category);

return name == category;

return bayType ? bayType->Contains(category) : name == category;

this would be my personal preference, do as you like

RisingLeaf · 2024-05-12T12:09:02Z

source/Ship.cpp

 	for(const Bay &bay : bays)
-		count += (bay.category == category);
+		if(bay.CanContain(category))
+			++count;


you could use a std::count_if

tibetiroka · 2024-05-16T07:04:01Z

source/BayType.cpp

+
+// Confirm that all categories of ship that this bay can hold
+// are carriable. If not, set isValid to false.
+void BayType::FinishLoading()


Would it make sense to ignore those categories instead of disabling the entire bay?

tibetiroka · 2024-05-16T07:20:16Z

tests/unit/src/test_ship.cpp

Some tests for the new feature could be useful. (Making sure it can load, detects when invalid categories are used, etc.)

Amazinite added 2 commits March 10, 2024 04:46

Allow the creation of bays that can carry ships of multiple types

28a8bc0

NPE fix

36663ea

Amazinite added the enhancement A suggestion for new content or functionality that requires code changes label Mar 10, 2024

Amazinite linked an issue Mar 10, 2024 that may be closed by this pull request

Allow bays to carry multiple categories of ship #9896

Open

Sort docking ships into bays instead of just finding the first availa…

f7f7558

…ble bay

Amazinite marked this pull request as ready for review March 12, 2024 08:14

RisingLeaf reviewed Mar 12, 2024

View reviewed changes

Apply suggestions from code review

5091977

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

TomGoodIdea reviewed Mar 29, 2024

View reviewed changes

source/BayType.cpp Outdated Show resolved Hide resolved

Koranir approved these changes Apr 21, 2024

View reviewed changes

source/Ship.cpp Outdated Show resolved Hide resolved

TomGoodIdea added the waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. label May 4, 2024

Amazinite and others added 2 commits May 11, 2024 16:02

Merge remote-tracking branch 'upstream/master' into 0.10.7-bay-types

0c817dd

Apply suggestions from code review

ce94192

Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com> Co-authored-by: Daniel <101683475+Koranir@users.noreply.github.com>

Amazinite added waiting on reviewer A Reviewer/Asignee needs to do something, e.g. reviewing, making suggestions, etc. and removed waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. labels May 11, 2024

Amazinite requested review from TomGoodIdea and RisingLeaf May 11, 2024 20:13

RisingLeaf reviewed May 11, 2024

View reviewed changes

source/Ship.cpp Outdated Show resolved Hide resolved

source/Ship.h Outdated Show resolved Hide resolved

source/GameData.h Outdated Show resolved Hide resolved

TomGoodIdea reviewed May 11, 2024

View reviewed changes

source/PlayerInfo.cpp Outdated Show resolved Hide resolved

Amazinite and others added 4 commits May 11, 2024 17:29

Change how defined bay types are accessed + move Ship::BayContains to…

30efb2b

… the namespace

Flip negation

ad6ec83

Because of course you should do the true case first Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>

Move the bay contains function to... Bay

f561448

Cache the bay type counts of a ship

aae2601

TomGoodIdea reviewed May 11, 2024

View reviewed changes

source/Ship.cpp Outdated Show resolved Hide resolved

source/Ship.h Outdated Show resolved Hide resolved

Do that thing

2b119ad

Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>

TomGoodIdea approved these changes May 12, 2024

View reviewed changes

source/Ship.cpp Outdated Show resolved Hide resolved

source/Ship.cpp Outdated Show resolved Hide resolved

bene-dictator removed the waiting on reviewer A Reviewer/Asignee needs to do something, e.g. reviewing, making suggestions, etc. label May 12, 2024

Death to unnecessary curly braces

5587933

Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>

RisingLeaf reviewed May 12, 2024

View reviewed changes

tibetiroka reviewed May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the creation of bays that can carry ships of multiple types #9906

Allow the creation of bays that can carry ships of multiple types #9906

Amazinite commented Mar 10, 2024 •

edited

Amazinite commented Mar 10, 2024 •

edited

tibetiroka commented Mar 10, 2024

tibetiroka commented Mar 10, 2024

Amazinite commented Mar 10, 2024

Amazinite commented Mar 10, 2024

Amazinite commented Mar 10, 2024

tibetiroka commented Mar 10, 2024 •

edited

Amazinite commented Mar 10, 2024

xX-Dillinger-Xx commented Mar 10, 2024 •

edited

Amazinite commented Mar 10, 2024

xX-Dillinger-Xx commented Mar 10, 2024 •

edited

Amazinite commented Mar 10, 2024

Amazinite commented Mar 12, 2024

RisingLeaf Mar 12, 2024

Amazinite Mar 16, 2024

RisingLeaf May 11, 2024

Koranir left a comment

RisingLeaf commented May 11, 2024

TomGoodIdea left a comment

RisingLeaf May 12, 2024

RisingLeaf May 12, 2024

RisingLeaf May 12, 2024

RisingLeaf May 12, 2024

RisingLeaf May 12, 2024

tibetiroka May 16, 2024

tibetiroka May 16, 2024

Allow the creation of bays that can carry ships of multiple types #9906

Are you sure you want to change the base?

Allow the creation of bays that can carry ships of multiple types #9906

Conversation

Amazinite commented Mar 10, 2024 • edited

Summary

Usage examples

Testing Done

Amazinite commented Mar 10, 2024 • edited

tibetiroka commented Mar 10, 2024

tibetiroka commented Mar 10, 2024

Amazinite commented Mar 10, 2024

Amazinite commented Mar 10, 2024

Amazinite commented Mar 10, 2024

tibetiroka commented Mar 10, 2024 • edited

Amazinite commented Mar 10, 2024

xX-Dillinger-Xx commented Mar 10, 2024 • edited

Amazinite commented Mar 10, 2024

xX-Dillinger-Xx commented Mar 10, 2024 • edited

Amazinite commented Mar 10, 2024

Amazinite commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koranir left a comment

Choose a reason for hiding this comment

RisingLeaf commented May 11, 2024

TomGoodIdea 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

Amazinite commented Mar 10, 2024 •

edited

Amazinite commented Mar 10, 2024 •

edited

tibetiroka commented Mar 10, 2024 •

edited

xX-Dillinger-Xx commented Mar 10, 2024 •

edited

xX-Dillinger-Xx commented Mar 10, 2024 •

edited