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.

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

Added dynamic railroad track terrain to Tiberian Sun #11774

Merged
merged 2 commits into from Apr 17, 2017

Conversation

Projects
None yet
4 participants
@Mailaender
Member

Mailaender commented Aug 6, 2016

Another small step towards #2652.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 6, 2016

Member

I'm not a big fan of having three completely separate implementations for doing the same thing (Bridge/LowBridge/BridgeLayer, LaysTerrain/BuildableTerrainLayer, and now ChangesTerrain).

There's got to be a better way forward than this...

Member

pchote commented Aug 6, 2016

I'm not a big fan of having three completely separate implementations for doing the same thing (Bridge/LowBridge/BridgeLayer, LaysTerrain/BuildableTerrainLayer, and now ChangesTerrain).

There's got to be a better way forward than this...

this.info = info;
}
void INotifyAddedToWorld.AddedToWorld(Actor self)

This comment has been minimized.

@pchote

pchote Aug 6, 2016

Member

You will need to clean up after yourself on removal, and think about what happens if CustomTerrain is already non-null.

@pchote

pchote Aug 6, 2016

Member

You will need to clean up after yourself on removal, and think about what happens if CustomTerrain is already non-null.

This comment has been minimized.

@Mailaender

Mailaender Aug 6, 2016

Member

Not for indestructible railways at the moment, but yes indeed later on that will be required.

@Mailaender

Mailaender Aug 6, 2016

Member

Not for indestructible railways at the moment, but yes indeed later on that will be required.

This comment has been minimized.

@pchote

pchote Aug 26, 2016

Member

This is a general purpose trait that will be available to all modders, so i'm not really happy with the idea of merging this with bugs.

@pchote

pchote Aug 26, 2016

Member

This is a general purpose trait that will be available to all modders, so i'm not really happy with the idea of merging this with bugs.

This comment has been minimized.

@Mailaender

Mailaender Aug 28, 2016

Member

You are right. This is an unnecessary limitation. Fixed.

@Mailaender

Mailaender Aug 28, 2016

Member

You are right. This is an unnecessary limitation. Fixed.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 26, 2016

Member

What about mobile units?
Also #11774 (comment):

[...] and think about what happens if CustomTerrain is already non-null.

Member

abcdefg30 commented Sep 26, 2016

What about mobile units?
Also #11774 (comment):

[...] and think about what happens if CustomTerrain is already non-null.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 26, 2016

Member

What about mobile units?

The Requires<ImmobileInfo> addresses that.

Member

pchote commented Sep 26, 2016

What about mobile units?

The Requires<ImmobileInfo> addresses that.

void INotifyRemovedFromWorld.RemovedFromWorld(Actor self)
{
var cell = self.Location;
var map = self.World.Map;

This comment has been minimized.

@pchote

pchote Sep 26, 2016

Member

These are redundant, just use them inline.

@pchote

pchote Sep 26, 2016

Member

These are redundant, just use them inline.

This comment has been minimized.

@Mailaender

Mailaender Oct 2, 2016

Member

They make the code easier to read for me.

@Mailaender

Mailaender Oct 2, 2016

Member

They make the code easier to read for me.

@LipkeGu

This comment has been minimized.

Show comment
Hide comment
@LipkeGu

LipkeGu Sep 29, 2016

Member

I'm not a big fan of having three completely separate implementations for doing the same thing >(Bridge/LowBridge/BridgeLayer, LaysTerrain/BuildableTerrainLayer, and now ChangesTerrain).

There's got to be a better way forward than this...

ChangeTerrainLayer for all would be good for all what uses the same logic

Member

LipkeGu commented Sep 29, 2016

I'm not a big fan of having three completely separate implementations for doing the same thing >(Bridge/LowBridge/BridgeLayer, LaysTerrain/BuildableTerrainLayer, and now ChangesTerrain).

There's got to be a better way forward than this...

ChangeTerrainLayer for all would be good for all what uses the same logic

@pchote

pchote approved these changes Apr 17, 2017

@pchote pchote merged commit 37219ad into OpenRA:bleed Apr 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Mailaender Mailaender deleted the Mailaender:changesterrain branch Apr 17, 2017

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