Skip to content

Destroy ships when player is defeated #1883

Open
Flamefire wants to merge 7 commits into
Return-To-The-Roots:masterfrom
Flamefire:sinkShipsOnDefeat
Open

Destroy ships when player is defeated #1883
Flamefire wants to merge 7 commits into
Return-To-The-Roots:masterfrom
Flamefire:sinkShipsOnDefeat

Conversation

@Flamefire
Copy link
Copy Markdown
Member

@Flamefire Flamefire commented Feb 2, 2026

As suggested in #183 ships will now be destroyed when a player is defeated or surrenders.

Requires:

@Spikeone
Copy link
Copy Markdown
Member

Spikeone commented Feb 2, 2026

Juast asking - this is not an addon, is it? I'm actually not sure if it should or not. Also if it should depend on if a player is defeated with destroying buildings or without. Guess this is just always which should be fine I guess, although this might stop ongoing sea attacks opposed to a normal attack which would still be going on.

@Flamefire
Copy link
Copy Markdown
Member Author

Flamefire commented Feb 3, 2026

This is not an addon. When a player is defeated (no more military, warehouses/HQs) or he surrenders his ships are destroyed/removed.

I don't see any issue with that and it's what I got out of #183

Thats also kinda important when you defeat an enemy and there are only ships left all over the coast(s).

For people etc. they will wander around and eventually die, so why not do the same for ships as everything else will be gone too.

@Flamefire Flamefire requested a review from Flow86 February 3, 2026 08:16
@Flow86
Copy link
Copy Markdown
Member

Flow86 commented Feb 7, 2026

Rebase necessary

@Flamefire
Copy link
Copy Markdown
Member Author

@Flow86 *ping

@Flamefire Flamefire force-pushed the sinkShipsOnDefeat branch from 01092c5 to 1fb8f3e Compare March 7, 2026 11:05
This is run for every gameframe in "real mode" and required for some
tests simulating e.g. a single step.
This is a corner case when destroying the harbor leads to defeat which
leads to sinking ships potentially with wares to this building.
Comment on lines +1515 to +1518
const auto shipsCopy = ships; // copy to avoid modification during iteration
for(auto* ship : shipsCopy)
ship->Sink();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't this be an addon? in the original RttR, defeated ships were not destroyed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Depends on - in the original only the player ever had ships, so it's a quite RttRish problem. Although I think just removing them is somewhat funny, since there is no burning/sinking animation. Maybe we should add some kind of animation (even the fire used for buildings does make sense I think).

Copy link
Copy Markdown
Member

@Flow86 Flow86 Apr 12, 2026

Choose a reason for hiding this comment

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

yep some burning/oil spill/whatever would be cool

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Using the small building fire as a vanishing, non-blocking object would work and not cause much trouble. Or do you want to make a new animation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if that works, I think thats a good thing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we don't need a new animation and would like the small building fire as you described it @Flamefire

Comment thread libs/s25main/nodeObjs/noShip.h Outdated
@Flow86
Copy link
Copy Markdown
Member

Flow86 commented Apr 12, 2026

I guess, although this might stop ongoing sea attacks opposed to a normal attack which would still be going on.

Well, this is maybe not wanted by default. If you loose your HQ, you still can reconquer stuff right? but I assume then you not count as "defeated" yet, right?

Co-authored-by: Flow86 <656249+Flow86@users.noreply.github.com>
@Flamefire
Copy link
Copy Markdown
Member Author

IIRC attacking soldiers get "cancelled" when their home building is destroyed, at least up to a certain point in their conquering.

"Defeated" means you have no more military buildings and no more storehouses.
But in any case: Even if they conquer anything the player cannot do anything else as he will never again have carriers or wares.

@Spikeone
Copy link
Copy Markdown
Member

Spikeone commented May 2, 2026

"Defeated" means you have no more military buildings and no more storehouses.
But in any case: Even if they conquer anything the player cannot do anything else as he will never again have carriers or wares.

Well yeah it's hard to know what defeated in most cases means. For me a player counts as defeated, when he lost control. This can be either giving up or by game goal. Players who have no storehouse left, can still attack and are maybe in terms of winnig often defeated, but they can still attack and annoy you. So only if the player is really defeated (lost control) then they should lose their ships I think.

@Flamefire
Copy link
Copy Markdown
Member Author

"Defeated" means you have no more military buildings and no more storehouses.
Players who have no storehouse left, can still attack and are maybe in terms of winnig often defeated, but they can still attack and annoy you.

That's our definition of defeated (in the code) so those players cannot attack you anymore. Hence it's safe to remove their ships, isn't it?

@Spikeone
Copy link
Copy Markdown
Member

Spikeone commented May 3, 2026

That's our definition of defeated (in the code) so those players cannot attack you anymore. Hence it's safe to remove their ships, isn't it?

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants