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.

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

Carryall edge spawn & Harvester delivery #7300

Closed

Conversation

MatthijsBenschop
Copy link
Contributor

This spawns the Carryall under either fog, shroud or -when that's not available- at the map edge.

It has already been debated whether that's the way to go, but looking at original d2k I really believe this is the best way.

Next PR would be harvester insurance (in case of worm frenzy)

@phrohdoh
Copy link
Member

phrohdoh commented Jan 9, 2015

Off topic but a nit: AutoCaryall should not be named so if you can configure it to not be automatic.

Inverting that logic to accompany the name Caryall would be better.

@MatthijsBenschop
Copy link
Contributor Author

@phrohdoh It's still an automated unit, it just doesn't get new orders automatically.

@pchote
Copy link
Member

pchote commented Jan 9, 2015

I remember making the same comment somewhere else. We're dealing with trait level names, and so if the trait is relying on another trait to give it orders then it is no longer automatic.

@@ -28,24 +30,25 @@ public class CarryUnit : Activity
readonly IPositionable positionable;
readonly IFacing cFacing; // Carryable facing
readonly IFacing sFacing; // Self facing
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having these comments I am fine with selfFacing and carryableFacing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not part of this PR but will do

@Mailaender
Copy link
Member

Also needs a rebase now.

@Mailaender
Copy link
Member

This has been silently updated.


namespace OpenRA.Mods.D2k.Traits
{
[Desc("Delivers a spice harvester after construction of this building. Optionally provides the harvester insurance function.")]
Copy link
Member

Choose a reason for hiding this comment

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

Optionally provides the harvester insurance function. is not quite there.

Copy link
Member

Choose a reason for hiding this comment

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

Delivers a spice harvester after construction of this building. should probably be Delivers a unit as this is far more general.

@Mailaender
Copy link
Member

This spawns the Carryall under either fog, shroud or -when that's not available- at the map edge.

It has already been debated whether that's the way to go, but looking at original d2k I really believe this is the best way.

This will look weird and unpolished in replays and for spectators with carryalls popping out of nowhere. Also the shellmap fog hack indicates that this behavior, although it tries to imitate the original 1:1, is not ideal for this engine.

@Mailaender
Copy link
Member

Otherwise this work really well! 👍

@penev92
Copy link
Member

penev92 commented Jan 17, 2015

I agree that map-edge-spawining would look better for both players and spectators, and also would be a lot easier on the CPU

@obrakmann
Copy link
Contributor

While I agree that only spawning at map edges would look better, it would also be bad for balance in some cases, especially in the early game when players would possibly have to wait considerably longer for their first harvester than their opponents. On maps such as "Imperial Basin" this would seriously handicap players that spawn in the middle. On the other hand, Imperial Basin is the only map we ship where that would really be an issue. Anyway, at the very least I think this aspect should be considered before asking for changes here. Let's find a consensus.

@MatthijsBenschop
Copy link
Contributor Author

Well.. I've already thought about making the under-shroud-spawn optional. Where normal maps will have edge spawn, but a map with custom yaml, or single player maps, could enable the option for shroud spawn.

This would apply to carryall production, harvester delivery and harvester insurance.

b.t.w. I'll try to finish this pr asap.. just very limited in spare time :/

@penev92
Copy link
Member

penev92 commented Jan 29, 2015

Let's find a consensus.

My vote is for edge-spawning (actually I'm OK with spawning new carryalls from the High-tech factory) for harvester delivery for the reasons mentioned above, despite the slight advantage (a matter of seconds) that would give some players on a couple of maps.

@Unit158
Copy link
Contributor

Unit158 commented Jan 30, 2015

It may be best to give carryalls a speedy spawn (much faster than missile speed, like spy plane) so the advantage is nullified. Either that or make them impossible to target, as was the case in Dune 2. The biggest potential problems with edge spawn are tanks or turrets sniping spawning carryalls.

On 2015-01-29, at 1:38 PM, penev92 notifications@github.com wrote:

Let's find a consensus.

My vote is for edge-spawning (actually I'm OK with spawning new carryalls from the High-tech factory) for harvester delivery for the reasons mentioned above, despite the slight advantage (a matter of seconds) that would give some players on a couple of maps.


Reply to this email directly or view it on GitHub.

@Micr0Bit
Copy link
Member

im fine with edge-spawning and the option for shroud-spawning

but i dont like carryall to spawn from a hightech factory ... or a speed-buff for spawning

@Mailaender
Copy link
Member

Needs a rebase.

@penev92
Copy link
Member

penev92 commented Feb 19, 2015

I'd really like to see this in the next playtest, as we really need the harvester insurance.

Code style and name changes
@pchote
Copy link
Member

pchote commented Mar 12, 2015

Activity has stalled here, so moving to the future milestone and closing for now.

@penev92
Copy link
Member

penev92 commented Mar 14, 2015

This has been superseded, so I'm removing the Future milestone.

@penev92 penev92 removed this from the Future milestone Mar 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants