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

Allow WithIdleOverlayInfo to render while the actor is being build. #14271

Merged
merged 1 commit into from Nov 19, 2017

Conversation

Projects
None yet
5 participants
@IceReaper
Contributor

IceReaper commented Oct 26, 2017

Allow WithIdleOverlayInfo to render while the actor is being build. This is usefull for various effects like "burning" or other effects which are also valid if the actor is still being build.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Oct 26, 2017

Contributor

A better option would be to remove the buildComplete checks entirely and just set up everything against WithMakeAnimation->Condition.

Contributor

GraionDilach commented Oct 26, 2017

A better option would be to remove the buildComplete checks entirely and just set up everything against WithMakeAnimation->Condition.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 26, 2017

Member

That's the best solution and the long (hopefully medium, after IDisable) plan. I suggested that @IceReaper take this approach for now, as it's not really fair to ask him to stop work on his mod until he rewrites the conditions setup in ra/cnc/d2k/ts. The flag can hopefully be removed again with no harm done before we ship the first playtest of the next release.

Member

pchote commented Oct 26, 2017

That's the best solution and the long (hopefully medium, after IDisable) plan. I suggested that @IceReaper take this approach for now, as it's not really fair to ask him to stop work on his mod until he rewrites the conditions setup in ra/cnc/d2k/ts. The flag can hopefully be removed again with no harm done before we ship the first playtest of the next release.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Oct 26, 2017

Contributor

You decided to encourage a precedent at #11093 - where the requirement became a Stance.Owner flag and I sidestepped with a downstream duplication instead - so I don't understand why you don't follow your own guidelines.

Contributor

GraionDilach commented Oct 26, 2017

You decided to encourage a precedent at #11093 - where the requirement became a Stance.Owner flag and I sidestepped with a downstream duplication instead - so I don't understand why you don't follow your own guidelines.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 26, 2017

Member

@GraionDilach: That pull request was a good example of bad communication sinking a reasonable feature. If you had initially clarified (or reopened the PR after you did clarify) that @abcdefg30's request would require a much wider code rework then i'd like to think that we would have responded with "oh whoops, good point, sorry" and moved forward to merge it with both a ValidStances enum for Ally/Neutral/Enemy and your flag for the owner -- mirroring the setup from weapons.

I believe that my comment three months later came from a context of an IRC discussion about whether mod dlls like OpenRA.Mod.AS should exist at all. I stand by my statement that mod authors should not be required to have all their custom code merged upstream.

Member

pchote commented Oct 26, 2017

@GraionDilach: That pull request was a good example of bad communication sinking a reasonable feature. If you had initially clarified (or reopened the PR after you did clarify) that @abcdefg30's request would require a much wider code rework then i'd like to think that we would have responded with "oh whoops, good point, sorry" and moved forward to merge it with both a ValidStances enum for Ally/Neutral/Enemy and your flag for the owner -- mirroring the setup from weapons.

I believe that my comment three months later came from a context of an IRC discussion about whether mod dlls like OpenRA.Mod.AS should exist at all. I stand by my statement that mod authors should not be required to have all their custom code merged upstream.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 19, 2017

Member

I have amended the PR to rename RenderIfBuildNotCompleteRenderBeforeBuildComplete as discussed in IRC.

Member

pchote commented Nov 19, 2017

I have amended the PR to rename RenderIfBuildNotCompleteRenderBeforeBuildComplete as discussed in IRC.

@pchote

pchote approved these changes Nov 19, 2017

@pchote pchote merged commit 25968ee into OpenRA:bleed Nov 19, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@IceReaper IceReaper deleted the IceReaper:WithIdleOverlayInfoWhileBuilding branch Apr 5, 2018

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