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

Fix editor crash #14997

Merged
merged 1 commit into from Apr 6, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Mar 25, 2018

Fixes #14995.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 25, 2018

An alternative approach that would be worth considering is to split out a ^BasePlayer the same way we do for the world, and avoid initializing these traits at all in the editor.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Mar 30, 2018

An alternative approach that would be worth considering is to split out a ^BasePlayer the same way we do for the world, and avoid initializing these traits at all in the editor.

Imho that would be the preferred approach, as just using default values when certain traits are null feels like a hack/workaround. I agree that we don't need to initialize traits like ConquestVictoryConditions in the map editor.
The second commit should be fine, however.

@reaperrr reaperrr force-pushed the reaperrr:fix-editor-crash branch from ceec3b1 to 62b0a65 Mar 30, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 30, 2018

Updated.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me. 👍

Do we want to merge this before we can turn off the update warnings on maps?

{
get
{
return "Map editor now requires an EditorPlayer to avoid loading all kinds of unnecessary player traits.\n";

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 30, 2018

Member

No \n at the end of the message, please.


public override IEnumerable<string> AfterUpdate(ModData modData)
{
yield return "The map editor now requires an EditorPlayer actor. " +

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 30, 2018

Member

You need to add a \n instead of the white space at the end here. Using + doesn't automatically add a line break. (In case you wanted a line break here, as this message is quite long?)


public override IEnumerable<string> AfterUpdate(ModData modData)
{
yield return "The map editor now requires an EditorPlayer actor. " +

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

Please define and toggle a bool so that this message is only reported the first time around (which is in the mod rules), instead of being repeated for all maps.

@reaperrr reaperrr force-pushed the reaperrr:fix-editor-crash branch 2 times, most recently from cd0b32c to 20ced1d Mar 30, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 30, 2018

Updated.

@@ -32,6 +32,7 @@ public class ConquestVictoryConditionsInfo : ITraitInfo, Requires<MissionObjecti
public class ConquestVictoryConditions : ITick, INotifyObjectivesUpdated
{
readonly ConquestVictoryConditionsInfo info;
readonly Player player;

This comment has been minimized.

@pchote

pchote Mar 30, 2018

Member

Do you expect this to ever be anything except self.Owner?

This comment has been minimized.

@reaperrr

reaperrr Mar 31, 2018

Author Contributor

No.

@reaperrr reaperrr force-pushed the reaperrr:fix-editor-crash branch from 20ced1d to 1dfdd67 Apr 1, 2018

Add EditorPlayer to all mods
To prevent the editor from loading unnecessary or even incompatible
player traits.

@reaperrr reaperrr force-pushed the reaperrr:fix-editor-crash branch from 1dfdd67 to c127d1d Apr 1, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Apr 1, 2018

Updated, dropped the ConquestVictoryConditions changes as discussed on IRC.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@reaperrr reaperrr merged commit 4c16e51 into OpenRA:bleed Apr 6, 2018

2 checks passed

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

@reaperrr reaperrr deleted the reaperrr:fix-editor-crash branch May 7, 2018

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