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

Added ScreenMap support for common sprite effects #13979

Merged
merged 5 commits into from Sep 17, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented Sep 3, 2017

This adds the ScreenMap plumbing for sprite IEffects (which can later be used for other IEffects as well) and adapts all the easy sprite effects.

The only one of real importance here will be SpriteEffect, as it's used in LeavesTrails, projectile sprite trails and explosions, as well as a few other places.

The other migrated effects will likely give no noticable performance gains, but I migrated them as well since they were similarly easy to adapt and just might save a tiny additional bit of render time here and there.

Note about effects not touched by this PR:

  • Support for IEffectAboveShroud effects would seemingly require some more changes to WorldRenderer (if at all possible) for presumably low gains (compared to SpriteEffect), so a decision and possible implementation is deferred to the future.
  • System-drawn effects like contrails and beams, as well as special cases like TeslaZaps, and therefore most projectiles will possibly require some more low-level preparations and for that reason are left to a later follow-up, although they are definitely desired due to expected gains comparable to (or larger than) SpriteEffect in mods with considerable use of these.

Supersedes #11676.
Partially adresses #9443.

@reaperrr reaperrr added this to the Next + 1 milestone Sep 3, 2017

Show outdated Hide outdated OpenRA.Game/Traits/World/ScreenMap.cs Outdated
@@ -20,5 +20,8 @@ public interface IEffect
IEnumerable<IRenderable> Render(WorldRenderer r);
}
// Identifier interface for effects that are added to ScreenMap
public interface ISpatiallyPartitionable { }

This comment has been minimized.

@pchote

pchote Sep 3, 2017

Member

How about an ISpatiallyPartitionedEffect : IEffect { } instead of requiring both?

@pchote

pchote Sep 3, 2017

Member

How about an ISpatiallyPartitionedEffect : IEffect { } instead of requiring both?

This comment has been minimized.

@reaperrr

reaperrr Sep 3, 2017

Contributor

Originally I was doing that, but that would then later force us to migrate all projectiles in one go. Might make sense anyway, just explaining my train of thought behind using a separate interface.

@reaperrr

reaperrr Sep 3, 2017

Contributor

Originally I was doing that, but that would then later force us to migrate all projectiles in one go. Might make sense anyway, just explaining my train of thought behind using a separate interface.

This comment has been minimized.

@pchote

pchote Sep 3, 2017

Member

Oh right, I didn't think about the IProjectile : IEffect.

@pchote

pchote Sep 3, 2017

Member

Oh right, I didn't think about the IProjectile : IEffect.

Show outdated Hide outdated OpenRA.Game/World.cs Outdated
Show outdated Hide outdated OpenRA.Game/World.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Cnc/Effects/SatelliteLaunch.cs Outdated
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 4, 2017

Contributor

Updated.

Contributor

reaperrr commented Sep 4, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 14, 2017

Contributor

Updated.

Contributor

reaperrr commented Sep 14, 2017

Updated.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 16, 2017

Member

Limiting the scope is a really good idea. 👍

Member

Mailaender commented Sep 16, 2017

Limiting the scope is a really good idea. 👍

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 16, 2017

Contributor

Updated.

Contributor

reaperrr commented Sep 16, 2017

Updated.

@Mailaender

Can't spot regressions in-game.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 17, 2017

Contributor

Updated.

Contributor

reaperrr commented Sep 17, 2017

Updated.

@pchote

pchote approved these changes Sep 17, 2017

@pchote pchote merged commit c3ece99 into OpenRA:bleed Sep 17, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Sep 17, 2017

Contributor

This crashes the game if a SpriteEffect starts with an empty frame.

Contributor

GraionDilach commented Sep 17, 2017

This crashes the game if a SpriteEffect starts with an empty frame.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 17, 2017

Contributor

This crashes the game if a SpriteEffect starts with an empty frame.

Are you sure that it's not #12115?

Contributor

reaperrr commented Sep 17, 2017

This crashes the game if a SpriteEffect starts with an empty frame.

Are you sure that it's not #12115?

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Sep 17, 2017

Contributor
Exception of type `System.ArgumentException`: Bounds of actor OpenRA.Mods.Common.Effects.SpriteEffect are empty.
Parameter name: bounds
   at OpenRA.Primitives.SpatiallyPartitioned`1.ValidateBounds(T actor, Rectangle bounds) in d:\OpenRA-AS-dev\OpenRA.Game\Primitives\SpatiallyPartitioned.cs:line 37
   at OpenRA.Primitives.SpatiallyPartitioned`1.Add(T item, Rectangle bounds) in d:\OpenRA-AS-dev\OpenRA.Game\Primitives\SpatiallyPartitioned.cs:line 42
   at OpenRA.Traits.ScreenMap.Add(IEffect effect, WPos position, Size size) in d:\OpenRA-AS-dev\OpenRA.Game\Traits\World\ScreenMap.cs:line 98
   at OpenRA.Traits.ScreenMap.Add(IEffect effect, WPos position, Sprite sprite) in d:\OpenRA-AS-dev\OpenRA.Game\Traits\World\ScreenMap.cs:line 104
   at OpenRA.Mods.Common.Effects.SpriteEffect..ctor(WPos pos, World world, String image, String sequence, String palette, Boolean visibleThroughFog, Boolean scaleSizeWithZoom, Int32 facing) in d:\OpenRA-AS-dev\OpenRA.Mods.Common\Effects\SpriteEffect.cs:line 36
   at OpenRA.Mods.Common.Warheads.CreateEffectWarhead.<>c__DisplayClass9.<DoImpact>b__7(World w) in d:\OpenRA-AS-dev\OpenRA.Mods.Common\Warheads\CreateEffectWarhead.cs:line 134
   at OpenRA.World.Tick() in d:\OpenRA-AS-dev\OpenRA.Game\World.cs:line 365
   at OpenRA.Game.InnerLogicTick(OrderManager orderManager) in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 605
   at OpenRA.Game.LogicTick() in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 629
   at OpenRA.Game.Loop() in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 759
   at OpenRA.Game.Run() in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 799
   at OpenRA.Program.Run(String[] args) in d:\OpenRA-AS-dev\OpenRA.Game\Support\Program.cs:line 136
   at OpenRA.Program.Main(String[] args) in d:\OpenRA-AS-dev\OpenRA.Game\Support\Program.cs:line 40

#12115 crashes earlier, during file loading. And I had the anim working beforehand this PR, for many, many months.

Contributor

GraionDilach commented Sep 17, 2017

Exception of type `System.ArgumentException`: Bounds of actor OpenRA.Mods.Common.Effects.SpriteEffect are empty.
Parameter name: bounds
   at OpenRA.Primitives.SpatiallyPartitioned`1.ValidateBounds(T actor, Rectangle bounds) in d:\OpenRA-AS-dev\OpenRA.Game\Primitives\SpatiallyPartitioned.cs:line 37
   at OpenRA.Primitives.SpatiallyPartitioned`1.Add(T item, Rectangle bounds) in d:\OpenRA-AS-dev\OpenRA.Game\Primitives\SpatiallyPartitioned.cs:line 42
   at OpenRA.Traits.ScreenMap.Add(IEffect effect, WPos position, Size size) in d:\OpenRA-AS-dev\OpenRA.Game\Traits\World\ScreenMap.cs:line 98
   at OpenRA.Traits.ScreenMap.Add(IEffect effect, WPos position, Sprite sprite) in d:\OpenRA-AS-dev\OpenRA.Game\Traits\World\ScreenMap.cs:line 104
   at OpenRA.Mods.Common.Effects.SpriteEffect..ctor(WPos pos, World world, String image, String sequence, String palette, Boolean visibleThroughFog, Boolean scaleSizeWithZoom, Int32 facing) in d:\OpenRA-AS-dev\OpenRA.Mods.Common\Effects\SpriteEffect.cs:line 36
   at OpenRA.Mods.Common.Warheads.CreateEffectWarhead.<>c__DisplayClass9.<DoImpact>b__7(World w) in d:\OpenRA-AS-dev\OpenRA.Mods.Common\Warheads\CreateEffectWarhead.cs:line 134
   at OpenRA.World.Tick() in d:\OpenRA-AS-dev\OpenRA.Game\World.cs:line 365
   at OpenRA.Game.InnerLogicTick(OrderManager orderManager) in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 605
   at OpenRA.Game.LogicTick() in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 629
   at OpenRA.Game.Loop() in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 759
   at OpenRA.Game.Run() in d:\OpenRA-AS-dev\OpenRA.Game\Game.cs:line 799
   at OpenRA.Program.Run(String[] args) in d:\OpenRA-AS-dev\OpenRA.Game\Support\Program.cs:line 136
   at OpenRA.Program.Main(String[] args) in d:\OpenRA-AS-dev\OpenRA.Game\Support\Program.cs:line 40

#12115 crashes earlier, during file loading. And I had the anim working beforehand this PR, for many, many months.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Sep 17, 2017

Contributor

It isn't that ticket for sure, Just reproduced the crash with an infantry where I used it's last, dummy frame for a WithDeathAnimation. It would be really nice if the stackdump throw out the image-sequence combinations for the modder to fix this locally atleast.

Contributor

GraionDilach commented Sep 17, 2017

It isn't that ticket for sure, Just reproduced the crash with an infantry where I used it's last, dummy frame for a WithDeathAnimation. It would be really nice if the stackdump throw out the image-sequence combinations for the modder to fix this locally atleast.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 17, 2017

Contributor

Then I guess Animation.Image represents only the first frame of an animation.
In that case we need to derive the largest frame somehow and take the bounds from that instead.

Contributor

reaperrr commented Sep 17, 2017

Then I guess Animation.Image represents only the first frame of an animation.
In that case we need to derive the largest frame somehow and take the bounds from that instead.

@reaperrr reaperrr deleted the reaperrr:effect-screenmap-rev3 branch Feb 22, 2018

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