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 projectiles to use IRulesetLoaded properly #15064

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
6 participants
@CDVoidwalker
Copy link
Contributor

CDVoidwalker commented Apr 17, 2018

No description provided.

@@ -82,6 +82,19 @@ public class Ruleset
}
}
}

var projectileLoaded = weapon.Value.Projectile as IRulesetLoaded<WeaponInfo>;

This comment has been minimized.

@pchote

pchote Apr 17, 2018

Member

weapon.Value.Projectile is an IProjectileInfo so this will always be null. Use IRulesetLoaded<IProjectileInfo> instead.

This comment has been minimized.

@GraionDilach

GraionDilach Apr 23, 2018

Contributor

This is definitely wrong though as-is, since this was the version @ABrandau was constantly crashing with, so this line is not functional. I'm quite certain it should be weapon.Value as IRulesetLoaded<WeaponInfo>; then.

There is a testcase in AS/Engine-ABrandau/OpenRA as WarheadTrailProjectile within their AS namespace.

This comment has been minimized.

@CDVoidwalker

CDVoidwalker Apr 26, 2018

Author Contributor
weapon.Value as IRulesetLoaded<WeaponInfo>;

Results in following error: "Cannot be converted to OpenRA.Traits.IRulesetloaded<OpenRA.GameRules.WeaponInfo>"

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:rulesetfix branch from f2e99b5 to 3a62f38 Apr 17, 2018

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented Apr 17, 2018

Applied review notes

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@ltem ltem added the PR: Needs +2 label Apr 22, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Apr 22, 2018

Warheads are loaded as WeaponInfo, so why do we want to load as IProjectileInfo here?

This also causes a contradiction in the YamlException ( "Projectile type {0}" but then returning weapon.Key).

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 22, 2018

Oh, I see. That was my bad then. This would have been clearer if we had an example of it being used.

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:rulesetfix branch from 3a62f38 to abebbf4 Apr 23, 2018

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented Apr 23, 2018

Rebased and reverted to use WeaponInfo

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Since I can't dismiss my own review, I will force-change it this way, because this as-is was tested out to be wrong.

@abcdefg30 abcdefg30 removed the PR: Needs +2 label Apr 25, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Apr 27, 2018

Funstuff, this was right for the entire time - in all our tests before, we had the projectile-validation block within the warhead-iterating validation block. You can guess my face atm.

@CDVoidwalker CDVoidwalker force-pushed the CDVoidwalker:rulesetfix branch from abebbf4 to 0cd13b7 Apr 27, 2018

@CDVoidwalker

This comment has been minimized.

Copy link
Contributor Author

CDVoidwalker commented Apr 27, 2018

Moved projectile check to be before warheads for cleanliness.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍, this time for real.

@reaperrr reaperrr merged commit 0c30a1d into OpenRA:bleed Apr 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.