Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove LegacyBridgeHut and LegacyBridgeLayer #20899

Open
wants to merge 6 commits into
base: bleed
Choose a base branch
from
Open

Remove LegacyBridgeHut and LegacyBridgeLayer #20899

wants to merge 6 commits into from

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Jun 6, 2023

These traits were marked for removal for quite some time.

While this PR doesn't fix bridges completely, it does unify a lot of the logic between first gen bridges and second gen.

@PunkPun
Copy link
Member Author

PunkPun commented Jun 6, 2023 •
edited

I recon it might be a good idea to completely ditch the manually defined offsets in favour of some automatic joining mechanism. They are a pain to set manually. Now implementing automatic joining

We might also want to instead of spawning bridge actors on bridge tiles, have bridge actors baked in into the map. Similar to how it is in TS.

@PunkPun PunkPun force-pushed the bridges branch 3 times, most recently from 76220ec to 843d8e5 Compare June 6, 2023 12:25
@PunkPun PunkPun mentioned this pull request Jun 11, 2023
Merged
@PunkPun PunkPun force-pushed the bridges branch 5 times, most recently from 03a0dac to 8eb62d1 Compare June 20, 2023 17:05
@PunkPun PunkPun force-pushed the bridges branch 5 times, most recently from 0595abf to 894e562 Compare August 8, 2023 13:05
@Mailaender
Copy link
Member

There is a bug where Tanya or the engineer get dragged away (walking through cliffs) in the opposite direction of the bridge:

image

@PunkPun
Copy link
Member Author

PunkPun commented Sep 22, 2023

#21071 this should fix that error

@PunkPun
Copy link
Member Author

PunkPun commented Sep 22, 2023

rebased

@Mailaender
Copy link
Member 

Exception of type `System.IndexOutOfRangeException`: Index was outside the bounds of the array.
   at OpenRA.Mods.Common.Traits.BridgeHut.NextNeighbourStep(IEnumerable`1 seed, HashSet`1 processed)+MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at OpenRA.Mods.Common.Traits.BridgeHut.<>c__DisplayClass13_0.<OpenRA.Traits.INotifyCreated.Created>b__0(World w) in OpenRA/OpenRA.Mods.Common/Traits/Buildings/BridgeHut.cs:line 87
   at OpenRA.World.Tick() in OpenRA/OpenRA.Game/World.cs:line 453
   at OpenRA.Game.InnerLogicTick(OrderManager orderManager) in OpenRA/OpenRA.Game/Game.cs:line 631
   at OpenRA.Game.LogicTick() in OpenRA/OpenRA.Game/Game.cs:line 646
   at OpenRA.Game.Loop() in OpenRA/OpenRA.Game/Game.cs:line 815
   at OpenRA.Game.Run() in OpenRA/OpenRA.Game/Game.cs:line 868
   at OpenRA.Game.InitializeAndRun(String[] args) in OpenRA/OpenRA.Game/Game.cs:line 305
   at OpenRA.Launcher.Program.Main(String[] args) in OpenRA/OpenRA.Launcher/Program.cs:line 32

when starting map Alpha Juno V on cnc.

@PunkPun
Copy link
Member Author

PunkPun commented Sep 22, 2023

The crash was cause by a bug in the original implementation

@PunkPun PunkPun force-pushed the bridges branch 2 times, most recently from 27a6f36 to 1e15c00 Compare September 22, 2023 16:03
@Mailaender
Copy link
Member

For ra the DemolishPropagationDelay is now 5. It was 20 before.

@PunkPun
Copy link
Member Author

PunkPun commented Sep 23, 2023

I see, it was using RepairPropagation as DemolishPropagation

@PunkPun
Copy link
Member Author

PunkPun commented Sep 23, 2023 •
edited

By slowing it down I noticed bugs in the detonation and repair order. In this PR bridge on polar disorder explodes (1, 1, 2, 2, 2) where the number refers to amount of segments exploding at an instance. While on bleed its (2, 1, 1, 1, 1, 1).

This is taking me a while to fix

OldBridge.mp4
NewBridge.mp4

RoosterDragon
RoosterDragon reviewed Nov 5, 2023
View reviewed changes
Copy link
Member

@RoosterDragon RoosterDragon left a comment •
edited

Choose a reason for hiding this comment

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

Plays fine. Can destroy and repair bridges without issues.

OpenRA.Mods.Common/Traits/Buildings/Bridge.cs
}

void IRadarSignature.PopulateRadarSignatureCells(Actor self, List<(CPos Cell, Color Color)> destinationBuffer)
{
destinationBuffer.AddRange(radarSignature);
}

string IBridgeSegment.Type => "GroundLevelBridge";
Copy link
Member

Choose a reason for hiding this comment

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

Consider extracting a constant for "GroundLevelBridge" to avoid string typing.

OpenRA.Mods.Common/Traits/World/BridgeLayer.cs Outdated Show resolved Hide resolved
pchote
pchote reviewed Dec 3, 2023
View reviewed changes
Copy link
Member

@pchote pchote left a comment •
edited

Choose a reason for hiding this comment

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

I noticed bugs in the detonation and repair order

I suspect this may be caused by the "Add automatic bridge segment joining and remove NeighbourOffsets" commit.

OpenRA.Mods.Common/Traits/Buildings/Bridge.cs
if (self.IsDead)
return;

// Use .FromPos since this actor is dead. Cannot use Target.FromActor.
Copy link
Member

Choose a reason for hiding this comment

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

The check immediately above suggests that this actor is not dead...?

pchote
pchote reviewed Dec 3, 2023
View reviewed changes
OpenRA.Mods.Common/Traits/World/LegacyBridgeLayer.cs
public override object Create(ActorInitializer init) { return new LegacyBridgeLayer(init.Self, this); }
}

public class LegacyBridgeLayer : IWorldLoaded
Copy link
Member

Choose a reason for hiding this comment

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

This commit also removes LegacyBridgeLayer.

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

@pchote pchote pchote left review comments

@Mailaender Mailaender Mailaender left review comments

@RoosterDragon RoosterDragon RoosterDragon left review comments

@penev92 penev92 penev92 left review comments

@abcdefg30 abcdefg30 abcdefg30 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
PR: Fixup requested PR: Rebase me!
Projects
None yet
Milestone
C&C Remastered Collection Support
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@PunkPun @Mailaender @pchote @RoosterDragon @penev92 @abcdefg30