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

Add several features to LaserZap #14988

Merged
merged 4 commits into from Jul 1, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Mar 24, 2018

Filing downstream changes from my MechWars mod.

  • beam source point now follows the source offset instead of being static
  • Added DamageInterval and DamageDuration for continous-damage support
  • Added LaunchEffect*, which in my opinion is more flexible and less issue-prone than WithMuzzleFlash and can be applied to other projectiles later
@@ -128,15 +156,15 @@ public void Tick(World world)
target = blockedPos;
}

if (!doneDamage)
if (ticks < info.DamageDuration && --interval <= 0)
{
if (hitanim != null)

This comment has been minimized.

@GraionDilach

GraionDilach Apr 4, 2018

Contributor

Do we really want the hitanim block into this if? I'd presume this block should be only ran in the first tick and not in all damaging ticks OR we want new hitanims played in all damaging ticks. This doesn't cover either.

@reaperrr reaperrr force-pushed the reaperrr:laserzap-updates branch 2 times, most recently from fcf244e to 1925101 Apr 26, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Apr 26, 2018

Updated.

HitAnim is now only played once (at first tick). Modders who want an impact animation every damage tick still have the option to use a CreateEffectWarhead for that.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Apr 26, 2018

LaunchEffect apparently lacks facing support.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Apr 26, 2018

LaunchEffect apparently lacks facing support.

Hm... the effect does, but not the implementation on this particular projectile.

I wrote this mostly with MechCommander/C&C3-style facing-less "glow" effects in mind, but I guess to qualify as WithMuzzleFlash replacement, facing-support is kinda mandatory. I'll see what I can do.

@reaperrr reaperrr force-pushed the reaperrr:laserzap-updates branch from 1925101 to ec1a08e Apr 26, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Apr 26, 2018

Updated with facing support, but haven't tested it yet.

The plumbing will later be used for other projectiles as well.

@@ -112,6 +119,8 @@ public LaserZap(LaserZapInfo info, ProjectileArgs args, Color color)

if (!string.IsNullOrEmpty(info.HitAnim))
hitanim = new Animation(args.SourceActor.World, info.HitAnim);
else
animationComplete = true;

This comment has been minimized.

@pchote

pchote Apr 29, 2018

Member

Can we rename this to something like showHitAnimation (and invert the value if needed)? Having a variable called animationComplete be true when there is no animation at all isn't very clear.

@@ -128,19 +137,19 @@ public void Tick(World world)
target = blockedPos;
}

if (!doneDamage)
if (ticks < info.DamageDuration && --interval <= 0)

This comment has been minimized.

@pchote

pchote Apr 29, 2018

Member

This doesn't seem to match the [Desc()] above: if DamageInterval is 5 and DamageDuration is 2 then I would expect ticks 0, 1 to deal damage, then ticks 5, 6, and so on. This looks like it will disable all damage after the first DamageDuration ticks.

This comment has been minimized.

@reaperrr

reaperrr Apr 29, 2018

Author Contributor

It works like I intended it to, seems the desc/property name aren't accurate enough then. Not sure what else to call it though, DamageTimeWindow?

This comment has been minimized.

@reaperrr

reaperrr May 12, 2018

Author Contributor

I settled for making the description more clear for now, since the behavior of DamageDuration matches the beam Duration (i.e. it stands for total time-frame, not a time-frame every n ticks).

}

public void Tick(World world)
{
source = args.CurrentSource();

if (hasLaunchEffect && ticks < info.LaunchEffectDuration)

This comment has been minimized.

@pchote

pchote Apr 29, 2018

Member

If info.LaunchEffectDuration > 1 then this will add a new effect every tick. I could understand this if the interval were configurable, but do we really have a use case for every tick?

This comment has been minimized.

@reaperrr

reaperrr Apr 29, 2018

Author Contributor

Well, I do in my mod, but I guess you're right that making the interval configurable would be better.

reaperrr added some commits Jul 6, 2017

Make LaserZap source point follow source actor muzzle
For moving actors, it looks better when the source point of the beam follows the source actor.

@reaperrr reaperrr force-pushed the reaperrr:laserzap-updates branch from ec1a08e to 2e6126b May 12, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 12, 2018

Updated.

@@ -25,6 +25,7 @@ public class ProjectileArgs
public int[] InaccuracyModifiers;
public int[] RangeModifiers;
public int Facing;
public Func<int> CurrentFacing;

This comment has been minimized.

@GraionDilach

GraionDilach May 24, 2018

Contributor

Why do you need to duplicate? Unless I'm overlooking something, we don't have a case where the initial facing should be still used after firing, so the Facing alone should be enough for being used for this.

This comment has been minimized.

@reaperrr

reaperrr May 24, 2018

Author Contributor

That's a good point, and any callers can still use Facing() if a static facing is needed.

@reaperrr reaperrr force-pushed the reaperrr:laserzap-updates branch from 2e6126b to cf6d1fd May 25, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 26, 2018

Updated.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 26, 2018

Wait a moment... why Facing() takes the firer's facing? That sounds wrong. It should always be used for the projectile's facing and lasers/waves/bolts/instahits can still get facings from the movement vector.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 26, 2018

If this is a huge feature-creeping though, feel free shifting out 7a6fd76 + that nit to a followup PR though.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 26, 2018

Um, first I'd have to understand what you mean, because currently I don't, sorry.

@@ -252,23 +252,23 @@ public virtual Barrel CheckFire(Actor self, IFacing facing, Target target)
protected virtual void FireBarrel(Actor self, IFacing facing, Target target, Barrel barrel)
{
Func<WPos> muzzlePosition = () => self.CenterPosition + MuzzleOffset(self, barrel);
var legacyFacing = MuzzleOrientation(self, barrel).Yaw.Angle / 4;
Func<int> legacyFacing = () => MuzzleOrientation(self, barrel).Yaw.Angle / 4;

This comment has been minimized.

@pchote

pchote May 26, 2018

Member

We need to be careful here, because this is changing semantics. Consider the case where a slow moving projectile is fired from an actor, which then turns after it has fired. It would be reasonable to expect args.Facing to not change in this case, because the facing of the projectile doesn't depend on that of the firing actor once it has been launched.

It might be clearer if we could instead expose this as a CurrentMuzzleFacing or similar to avoid confusion?

@reaperrr reaperrr force-pushed the reaperrr:laserzap-updates branch from cf6d1fd to f79ce04 May 26, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 26, 2018

Updated, split the facing changes to #15173.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 3, 2018

Can you please put together a testcase that copies some of the laser setups from your mod into RA or TD to demonstrate and test these features?


public IEnumerable<IRenderable> Render(WorldRenderer wr)
{
if (world.FogObscures(posFunc()))

This comment has been minimized.

@pchote

pchote Jun 3, 2018

Member

We can't trust posFunc to not do something arbitrarily weird between invocations. It would be safer to evaluate this in Tick and then save the result in a variable for use in the ScreenMap update and in rendering.

public readonly int LaunchEffectDuration = 1;

[Desc("Launch effect is spawned every this many ticks during LaunchEffectDuration.")]
public readonly int LaunchEffectInterval = 1;

This comment has been minimized.

@pchote

pchote Jun 3, 2018

Member

I still don't understand the purpose of these two fields - it doesn't make sense to me why you would want to repeatedly spawn the same effect for only part of a weapon's firing duration. If this is to implement a warmup-loop-cooldown cycle (like fire-start and fire-loop on the oil pump destruction) then that would be cleaner and certainly better for performance to define these explicitly in the LaunchEffect.

A demonstration/testcase for this would help a lot.

This comment has been minimized.

@reaperrr

reaperrr Jun 3, 2018

Author Contributor

I could do that, but - apart from not being easy to set up (don't have any suitable example in shp(td) or shp(ts) format) - by now I suspect it would get rejected anyway, for being aimed at a too specific scenario.

The use-case in my mod is that the laser muzzle effect is too "weak" with higher translucency, and still so-so (and too pixelated/aliased!) with very low or disabled translucency, so just prolonging the sequence doesn't cut it.
I tried all kinds of blend-modes, but using higher translucency + spawning another muzzle every tick for the LaunchEffectDuration is still by far producing the best-looking result (I even use a LaunchEffectIntensity that allows to spawn more than one effect per tick, which I didn't bother submitting as it's even more limited-scope).

I admit I can't think of any real uses outside that very specific use-case, it was more a case of "might as well submit them upstream along with the generally useful stuff, to reduce my local diff and in case anyone else has a use for this".
It's probably better to just drop them from this PR (I have to use a custom engine anyway, due to other changes).

This comment has been minimized.

@pchote

pchote Jun 3, 2018

Member

Ah, I see. So you're relying on stacking many overlapping sprites in order to build up the strength of the effect?
We had the same problem in D2K, which I worked around by adding the PaletteFromScaledPalette trait to increase the contrast in the palette itself - mimicking the effect of stacking multiple sprites but without the stacking.

This comment has been minimized.

@reaperrr

reaperrr Jun 3, 2018

Author Contributor

Tested that and works well enough for me. I'll drop these properties, then.

This comment has been minimized.

@pchote

pchote Jun 3, 2018

Member

Awesome, I'm glad we could resolve this in a way that improves things for everyone.

reaperrr added some commits Jul 6, 2017

Add LaunchEffect support to LaserZap
Can act as replacement for WithMuzzleOverlay.

@reaperrr reaperrr force-pushed the reaperrr:laserzap-updates branch from f79ce04 to 04e4c3a Jun 3, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 3, 2018

Updated.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Still 👍 from me.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Jul 1, 2018

This doesn't regress anything. ☑️

@Mailaender Mailaender merged commit f85f2cd into OpenRA:bleed Jul 1, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

Mailaender commented Jul 1, 2018

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.