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

Add support for arbitrary objective type names #16277

Merged
merged 2 commits into from Apr 22, 2019

Conversation

@abcdefg30
Copy link
Member

commented Mar 7, 2019

First commit splits an INotifyWinstateChanged interface from INotifyObjectivesUpdated which just reduces the amount of empty methods and unnecessary calls to traits implementing the latter.

Testcase: Start Allies01, you should see announcements for the usual objectives and the fake objective, but not the tertiary one (no blinking of the objectives icon, too, once the tertiary is added 7 seconds into the map). The tertiary objective will appear in the list once it is completed and the mission won't fail when the fake objective is marked as failed (both 17 seconds in).

@abcdefg30 abcdefg30 force-pushed the abcdefg30:objectives branch from 4c71487 to 96c8a93 Mar 7, 2019

@abcdefg30 abcdefg30 changed the title Add tertiary and fake primary objectives Add support for arbitrary objective names and hidden objectives Mar 7, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Updated as discussed on IRC. This gives more freedom to mappers now.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

I'll need to go all missions again in the foreseeable future anyway, so I'll replace AddPrimaryObjective and AddSecondaryObjective at the that time and only then mark those methods as deprecated.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:objectives branch 2 times, most recently from 3db280b to 9b200ad Mar 7, 2019

@pchote pchote requested a review from obrakmann Mar 16, 2019

@obrakmann
Copy link
Contributor

left a comment

Looks good overall. I'd just like to see how a proper implementation of the GDI06 special case would look.

// Work around the loop closure issue in older versions of C#
var objective = o;

if (objective.Hidden && objective.State == ObjectiveState.Incomplete)

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 17, 2019

Contributor

I'm thinking of the GDI06 case now. Do we need the ability to hide an objective even after it is completed? That would also mean suppressing the notifications for it, for example.

In fact I would love if this PR would use GDI06 as a test-case instead of a throw-away, because that mission is the main motivation to have this in the first place.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Apr 7, 2019

Author Member

This does not make sense to me. If you want completed objectives hidden you might as well use boolean variables. What use case do you have in mind for objectives that never appear?

This comment has been minimized.

Copy link
@pchote

pchote Apr 7, 2019

Member

Do we need hidden objectives at all? Why can't we add the objective and then immediately complete/fail it when the necessary condition (probably a trigger) is set?

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Apr 7, 2019

Author Member

The case coming to my mind is having a completed primary objective, but the mission not ending yet. On the other hand, I think this can be worked around now that the type names are arbitrary. I'll remove the hidden type if nobody has a specific case for it.

This comment has been minimized.

Copy link
@pchote

pchote Apr 14, 2019

Member

We probably need to clarify / argue about what the desired behaviour for GDI06 actually is.

IMO it's confusing and wrong to have an objective "Destroy the Nod ********." marked as failed if there is no way for the player to know what ******** actually is during the mission.

We could implement a new API to replace the objective (or maybe just its text) so that ******** becomes Airfield, but i'm not convinced that this is better than always marking it completed, then leaving it to the next mission's intro to reveal that they made the wrong choice.

So my preference is to keep GDI06 as it currently is on bleed...

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2019

Updated the testcase (GDI06).

@abcdefg30 abcdefg30 force-pushed the abcdefg30:objectives branch from f551812 to b4ebf36 Apr 13, 2019

@abcdefg30 abcdefg30 changed the title Add support for arbitrary objective names and hidden objectives Add support for arbitrary objective type names Apr 13, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

Updated. Removed the special cased hidden type.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

My only concern here is with the final commit as mentioned above, so if we can move that into its own PR then I'll happily 👍 the substantive changes here.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Split the requested testcase to #16429.

@pchote
pchote approved these changes Apr 22, 2019

@pchote pchote added the PR: Needs +2 label Apr 22, 2019

@reaperrr reaperrr merged commit b2278e8 into OpenRA:bleed Apr 22, 2019

2 checks passed

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

@abcdefg30 abcdefg30 deleted the abcdefg30:objectives branch Apr 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.