HomeWildfire Games

Move more things in the projectile part of the Attack/Ranged component
AuditedrP22184

Description

Move more things in the projectile part of the Attack/Ranged component

Following D945, I reckon more things should be moved from Attack/Ranged to Attack/Ranged/Projectile.

In the long run, I think most things should be moved to the Projectile, which would let us have several projectiles per attack (for example), make it easier to abstract away from the Melee/Ranged attack types, and would also be more coherent. This is the first step, as it adds the Projectile to all templates.

This also makes the launch point a parameter, though I only add "y" as implementing the rotation is left as an exercise to the reader.

Reviewed By: bb

Differential Revision: https://code.wildfiregames.com/D1171

Details

Event Timeline

Stan added a subscriber: Stan.Apr 13 2019, 11:37 AM
Stan added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/Attack.js
537

These three could be inlined no ?

Nescio added a subscriber: Nescio.Apr 13 2019, 11:48 AM

What does <LaunchPoint y="3"/> mean? Also, y suggests there could be x and z components.
Furthermore, if it's always 3, wouldn't it be better to set that by default? Same question for <Gravity>9.81</Gravity>.

This also makes the launch point a parameter, though I only add "y" as implementing the rotation is left as an exercise to the reader.

As commented, left as an exercise to the reader. The math isn't trivial.

wraitii added a subscriber: Itms.Apr 13 2019, 12:07 PM

@Itms Any idea why this failed? Compiled fine and ran on my machine.

Itms added a comment.EditedApr 13 2019, 12:24 PM

@Itms Any idea why this failed? Compiled fine and ran on my machine.

Yes, but you forgot to run the tests... ?

ERROR: JavaScript error: simulation/components/tests/test_Attack.js line 79
SyntaxError: missing } after property list

I'm sorry I'm not sure how to better display the tests output in non-differential builds. The contents of the test output will be printed in the Jenkins summary upon failure from now on. It's ugly because they are formatted as XML for automated handling, but it can be useful.

Itms added a comment.Apr 13 2019, 12:26 PM

As commented, left as an exercise to the reader. The math isn't trivial.

If the math isn't trivial, it should be done somewhere, so people don't have to rely on trusting you (especially in a few years when they stumble on the code).

Itms added a comment.Apr 13 2019, 12:31 PM
In rP22184#32826, @Itms wrote:

If the math isn't trivial, it should be done somewhere, so people don't have to rely on trusting you (especially in a few years when they stumble on the code).

Ah no sorry you're saying you are not implementing the rotation because it's convoluted and not worth doing the math. You should say that instead of writing "left as an exercise to the reader", which is 100% guaranteed to puzzle/make suspicious/annoy/infuriate people. ?

Yes, but you forgot to run the tests... ?

That doesn't sound like me at all :p

Ah no sorry you're saying you are not implementing the rotation because it's convoluted and not worth doing the math

Correct, I thought this was the obvious meaning of "left as an exercise to the reader" but obviously not :)

I'll fix the tests.

Stan raised a concern with this commit.Sep 24 2019, 3:40 PM

See D2326

/ps/trunk/binaries/data/mods/public/simulation/components/Attack.js
546

Missing cast ex:

WARNING: With cast: ({x:0, y:3, z:0})
WARNING: Without cast: ({x:0, y:"3", z:0})
This commit now has outstanding concerns.Sep 24 2019, 3:40 PM
Stan accepted this commit.Sep 25 2019, 8:53 AM

Fixed in rP22990

All concerns with this commit have now been addressed.Sep 25 2019, 8:53 AM