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

Tooltip fixes for the Parabombs #14414

Merged
merged 2 commits into from Dec 12, 2017

Conversation

Projects
None yet
5 participants
@Arular101
Contributor

Arular101 commented Nov 24, 2017

I noticed for the parabombs in Fort Lonestar, the Badger bomber was in the description instead of the MiG bomber. Also, a CameraRemoveDelay is added for that.

1
I removed the \n because there was no spacing between the title and the time behind it.

I reverted my previous change (Three Badgers instead of A Badger) because this turned out incorrect.

In the second commit I added in the description of the airfield the Parabomb special ability. I wanted to do that with a Buildable@UKRAINE: to have that description only for Ukraine. Sadly after various attempts, this didn't work and I get an exception log: Exception of type System.InvalidOperationException: TypeDictionary contains multiple instances of type OpenRA.Mods.Common.Traits.BuildableInfo
I changed it for now like this:
2
But this can be seen on any airfield. I would like to know the opinion of you guys if this is really wanted. If not, I could drop this commit.

A possible solution is to make a new AFLD.Ukraine, like how the British spy was made. But this is maybe a too big of a change for only the tooltip.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 24, 2017

Member

👍 for AFLD.Ukraine. You don't need to duplicate the AFLD yaml, just inherit and override it. You will also need to add an override for selection grouping - refer to the starport variants of the d2k vehicles, which do the same thing.

Member

pchote commented Nov 24, 2017

👍 for AFLD.Ukraine. You don't need to duplicate the AFLD yaml, just inherit and override it. You will also need to add an override for selection grouping - refer to the starport variants of the d2k vehicles, which do the same thing.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 24, 2017

Contributor

Updated with AFLD.Ukraine.

Sorry, I didn't understand what you mean with the override for selection grouping.

Contributor

Arular101 commented Nov 24, 2017

Updated with AFLD.Ukraine.

Sorry, I didn't understand what you mean with the override for selection grouping.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 24, 2017

Member

See e.g.

Selectable:
Class: trike

Add Class: afld to the airfield (which will then be inherited by afld.ukraine) to allow them to be grouped together by the "select all by type" hotkey.

Member

pchote commented Nov 24, 2017

See e.g.

Selectable:
Class: trike

Add Class: afld to the airfield (which will then be inherited by afld.ukraine) to allow them to be grouped together by the "select all by type" hotkey.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 25, 2017

Contributor

Updated. Thanks!

Contributor

Arular101 commented Nov 25, 2017

Updated. Thanks!

@@ -1274,12 +1274,14 @@ AFLD:
Buildable:
Queue: Building
BuildPaletteOrder: 130
Prerequisites: dome, ~structures.soviet, ~techlevel.medium
Prerequisites: dome, ~structures.soviet, ~techlevel.medium, ~!structures.ukraine

This comment has been minimized.

@pchote

pchote Dec 10, 2017

Member

The parabombs are tied to aircraft.ukraine, so I think it would be better to use that here than structures.ukraine.

Edit: scratch that, the prereq is granted by this structure so isn't going to work.

@pchote

pchote Dec 10, 2017

Member

The parabombs are tied to aircraft.ukraine, so I think it would be better to use that here than structures.ukraine.

Edit: scratch that, the prereq is granted by this structure so isn't going to work.

@@ -297,7 +297,7 @@ powerproxy.parabombs:
AirstrikePower:
Icon: parabombs
Description: Parabombs (Single Use)
LongDesc: Three Badgers drops a load of parachuted\nbombs on your target.
LongDesc: A Badger drops a load of parachuted bombs on your target.

This comment has been minimized.

@pchote

pchote Dec 10, 2017

Member

IMO it would make more sense (in a followup PR to avoid scope creeping this one) to fix this to use three planes like the main power.

@pchote

pchote Dec 10, 2017

Member

IMO it would make more sense (in a followup PR to avoid scope creeping this one) to fix this to use three planes like the main power.

This comment has been minimized.

@Arular101

Arular101 Dec 12, 2017

Contributor

I'm interested to make this change.

@Arular101

Arular101 Dec 12, 2017

Contributor

I'm interested to make this change.

@pchote pchote added this to the Next release milestone Dec 10, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 10, 2017

Member

Adding to the milestone because this is an easily reviewable polish win that fits in with the other tooltip cleanups.

Member

pchote commented Dec 10, 2017

Adding to the milestone because this is an easily reviewable polish win that fits in with the other tooltip cleanups.

@pchote pchote added the PR: Needs +2 label Dec 10, 2017

@Smittytron

At the risk of sounding pedantic, the description for AirstrikePower@parabombs in structures.yaml can be changed from 'drops' to 'drop'.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Dec 11, 2017

Member

I count 2 approving votes and a requested fixup.

Member

penev92 commented Dec 11, 2017

I count 2 approving votes and a requested fixup.

Parabombs tooltip for airfield
with selection grouping
@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Dec 12, 2017

Contributor

Updated, good catch.

Contributor

Arular101 commented Dec 12, 2017

Updated, good catch.

@abcdefg30 abcdefg30 merged commit a1b3fe2 into OpenRA:bleed Dec 12, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Dec 12, 2017

@Arular101 Arular101 deleted the Arular101:tooltipfix2 branch Dec 18, 2017

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