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

Fixed issue with unit ready when capturing #14220

Merged
merged 1 commit into from Jan 1, 2018

Conversation

Projects
None yet
6 participants
@AndriiYukhymchak
Contributor

AndriiYukhymchak commented Oct 19, 2017

Fixes #14219 and #13983 , hopefully without breaking anything else

Issue was with Actor.ChangeOwner being always scheduled last (since it was using World.AddFrameEndTask inside World.AddFrameEndTask) Modified the code so it now accepts current frame as an optional parameter and moved the actual transition of ownership to the bottom of the code.

Now... I don't know if this is the best solution, but it feels that way. Maybe we can move more actions inside of an Actor to proper "chain" from World.AddFrameEndTask. Maybe all this is a bogus idea, and should be denied

Either way - this fixes the issue, and initial tests shown that everything else works

Show outdated Hide outdated OpenRA.Game/Actor.cs Outdated
Show outdated Hide outdated OpenRA.Game/Actor.cs Outdated
Show outdated Hide outdated OpenRA.Game/Actor.cs Outdated
@netnazgul

This comment has been minimized.

Show comment
Hide comment
@netnazgul

netnazgul Oct 20, 2017

Contributor

So, now captured player will be punished twice - losing his building and producing a unit for enemy?

Also, what if player has multiple production buildings of that type and switches priority before the unit is finished?

Overall I'd suggest just destroying unit whatsoever instead of transferring ownership.

Contributor

netnazgul commented Oct 20, 2017

So, now captured player will be punished twice - losing his building and producing a unit for enemy?

Also, what if player has multiple production buildings of that type and switches priority before the unit is finished?

Overall I'd suggest just destroying unit whatsoever instead of transferring ownership.

@AndriiYukhymchak

This comment has been minimized.

Show comment
Hide comment
@AndriiYukhymchak

AndriiYukhymchak Oct 20, 2017

Contributor

@netnazgul it does not give unit to the player. If that was your last Barr or WF you lose the unit and unit queue, but refund your money. If this is not your only production building of this type, most likely you already produced it from another building

Contributor

AndriiYukhymchak commented Oct 20, 2017

@netnazgul it does not give unit to the player. If that was your last Barr or WF you lose the unit and unit queue, but refund your money. If this is not your only production building of this type, most likely you already produced it from another building

@netnazgul

This comment has been minimized.

Show comment
Hide comment
@netnazgul

netnazgul Oct 20, 2017

Contributor

Totally fine then

Contributor

netnazgul commented Oct 20, 2017

Totally fine then

@AndriiYukhymchak

This comment has been minimized.

Show comment
Hide comment
@AndriiYukhymchak

AndriiYukhymchak Dec 26, 2017

Contributor

can someone review this? @pchote @penev92 @MustaphaTR

Contributor

AndriiYukhymchak commented Dec 26, 2017

can someone review this? @pchote @penev92 @MustaphaTR

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Dec 27, 2017

Contributor

See #11987 (comment) why this is a bad idea.

In any case, the issues feel like sideeffects of that goddamned hardcoded lock mechanism and that should be axed in favor of conditions or something, the power refactor should have brought in the necessary prerequisites.

Contributor

GraionDilach commented Dec 27, 2017

See #11987 (comment) why this is a bad idea.

In any case, the issues feel like sideeffects of that goddamned hardcoded lock mechanism and that should be axed in favor of conditions or something, the power refactor should have brought in the necessary prerequisites.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 29, 2017

Member

IMO that example actually demonstrates why this is a good idea.

Right now ExternalCapture/Production is not robust against the order that notifications are called, which leads to #14219. A less invasive mod-level fix would be to wrap everything after the ChangeOwner in another FrameEndTask, but the fix here makes for cleaner code and logic. Because we are already inside a FrameEndTask there is no need to add an extra level or indirection.

Member

pchote commented Dec 29, 2017

IMO that example actually demonstrates why this is a good idea.

Right now ExternalCapture/Production is not robust against the order that notifications are called, which leads to #14219. A less invasive mod-level fix would be to wrap everything after the ChangeOwner in another FrameEndTask, but the fix here makes for cleaner code and logic. Because we are already inside a FrameEndTask there is no need to add an extra level or indirection.

@pchote pchote modified the milestone: Next release Dec 29, 2017

@pchote

LGTM, just two minor style nits.

To other reviewers: Add ?w=1 to the url to see that this is doesn't change anything outside the ExternalCapture case.

Show outdated Hide outdated OpenRA.Game/Actor.cs Outdated
Show outdated Hide outdated OpenRA.Game/Actor.cs Outdated
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 29, 2017

Member

This is a pretty straightforward bug fix, so i'd like to get this in for the playtest. I won't add it to the milestone though, as I don't want to force delaying that any longer.

Member

pchote commented Dec 29, 2017

This is a pretty straightforward bug fix, so i'd like to get this in for the playtest. I won't add it to the milestone though, as I don't want to force delaying that any longer.

@pchote

pchote approved these changes Jan 1, 2018

I pushed a fixup with my requests above. LGTM now

@GraionDilach

Ah, I can see now why this will work - that call was already within a FrameEndTask by then.

LGTM.

@pchote pchote merged commit 11db40a into OpenRA:bleed Jan 1, 2018

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Jan 2, 2018

Member

Errm, isn't this PR meant to cancel the unit when the capture is done. Not tested on main mods yet, but on my mod (rebased it to bleed today), i'm getting enemy units while tring to capture them. Maybe related to ClassicProduction -> NormalProduction. I'll test on RA too.

Member

MustaphaTR commented Jan 2, 2018

Errm, isn't this PR meant to cancel the unit when the capture is done. Not tested on main mods yet, but on my mod (rebased it to bleed today), i'm getting enemy units while tring to capture them. Maybe related to ClassicProduction -> NormalProduction. I'll test on RA too.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Jan 2, 2018

Member

Can not confirm on RA, but TD (after changing capturing to External). Looks like indeed something to do with Normal Production.

Member

MustaphaTR commented Jan 2, 2018

Can not confirm on RA, but TD (after changing capturing to External). Looks like indeed something to do with Normal Production.

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