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

Cargo crash fix and improvements #16295

Merged
merged 1 commit into from Mar 26, 2019

Conversation

@CDVoidwalker
Copy link
Contributor

commented Mar 10, 2019

This resolves issue #15261 and allows soft forks to add garrison cleaning like traits without crashing the game

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

@wolfbyte is currently preparing a PR for the garrisoning feature, so it would be wise to either wait for that or for confirmation that this will not get in the way of that PR.

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

I strictly done it just because I asked Mustapha if there's something to be done for RV and it seems to work on their repo as well

@wolfbyte

This comment has been minimized.

Copy link

commented Mar 10, 2019

This won't affect garrison. I'll apply this to Garrison's version of cargo when I send the PR.

@MustaphaTR

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I strictly done it just because I asked Mustapha if there's something to be done for RV and it seems to work on their repo as well

Btw, i didn't actually test it, i just looked at the code and seemed good to me, the will do now.

@MustaphaTR

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

It fixes the crash in RV, tho i feel like the condition revorking needs to be adjusted too so, it'll revoke the correct condition and not the one granted the last.

@abcdefg30
Copy link
Member

left a comment

Fix confirmed.

OpenRA.Mods.Common/Traits/Passenger.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Cargo.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Cargo.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Passenger.cs Outdated Show resolved Hide resolved

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:cargorefactor branch from 0cef355 to 6d9c943 Mar 22, 2019

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Applied review notes

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:cargorefactor branch from 6d9c943 to 80c6c74 Mar 22, 2019

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:cargorefactor branch from 80c6c74 to 18ac24d Mar 22, 2019

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:cargorefactor branch from 18ac24d to b511a1e Mar 26, 2019

@obrakmann
Copy link
Contributor

left a comment

Just one minor quibble, looks otherwise good to me. 👍

var a = cargo.Pop();
passenger = passenger ?? cargo.Last();
if (!cargo.Remove(passenger))
throw new NullReferenceException("Attempted to unload non existing actor from Cargo!");

This comment has been minimized.

Copy link
@obrakmann

obrakmann Mar 26, 2019

Contributor

This is just nit-picking now, but I'd prefer a throw new ArgumentException("Attempted to unload an actor that is not a passenger.");. It's a bit closer to the truth.

This comment has been minimized.

Copy link
@CDVoidwalker

CDVoidwalker Mar 26, 2019

Author Contributor

yeah sounds better, will push a fix in a second

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:cargorefactor branch from b511a1e to 72ff0d9 Mar 26, 2019

@pchote
pchote approved these changes Mar 26, 2019
Copy link
Member

left a comment

Code changes look reasonable now, but not tested.

@obrakmann obrakmann merged commit a46ec5d into OpenRA:bleed Mar 26, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

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