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 for V2 Rocket Launcher facings #14393

Merged
merged 1 commit into from Nov 26, 2017

Conversation

Projects
None yet
7 participants
@Arular101
Contributor

Arular101 commented Nov 20, 2017

I’ve found a solution for the V2 rocket launcher shaking. The V2 rocket launcher has 32 facings for when it is loaded with a rocket and 32 facings without a rocket. Additional, there are only 8 facings for when the rocket is raised and only 8 facings when the rocket is launched with that thing that holds the rocket still raised. Now, I’ve inspected the original game and that thing that raises the rocket is not used there. (I always thought that I missed that detail somehow in the originals.)

So, in order to fix this problem, I removed the aim and empty-aim in sequences/vehicles.yaml. Now the V2 rocket launcher doesn’t shake anymore. There is only a small problem with that.

If the V2 faces one direction (let’s say to the east) with still a rocket on it. And it shoot its rocket to the east, the image of the V2 doesn’t change to an image of a V2 without a rocket.
However, if the V2 moves after that, the image of the V2 gets updated and is now displayed without a rocket.

The solution for that was to reuse the empty-aim trait, but this time pointing at the 32 facings of a V2 without a rocket. Now the V2 rocket launcher works perfectly!

Closes #12117

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 20, 2017

Contributor

Whoops, Travis showed that the aim trait is needed. Added it back but now pointing it to the 32 facings of a V2 rocket launcher with a rocket.

Contributor

Arular101 commented Nov 20, 2017

Whoops, Travis showed that the aim trait is needed. Added it back but now pointing it to the 32 facings of a V2 rocket launcher with a rocket.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 20, 2017

Member

If we really wanna remove aiming graphics a better solution would be use Ammo, Conditions and 2 WithSpriteBody s and get rid of WithAttackAnimation imo.

Member

MustaphaTR commented Nov 20, 2017

If we really wanna remove aiming graphics a better solution would be use Ammo, Conditions and 2 WithSpriteBody s and get rid of WithAttackAnimation imo.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 21, 2017

Contributor

First of all, I agree with the general fix; the idea behind the aim sequences wasn't bad, but with only 8 facings, the glitching always hurt more than the raised rocket/ramp added. This should've been either upgraded to a full 32-facing sprite, or removed again long ago.

If we really wanna remove aiming graphics a better solution would be use Ammo, Conditions and 2 WithSpriteBody s and get rid of WithAttackAnimation imo.

This would be the best solution, yes.

But regardless of which approach, the "aim" and "aim-empty" sequences should go away now instead of becoming duplicates, WithAttackAnimation can use idle(-empty) as well.

@Arular101 If you need help with setting up the AmmoPool approach, notify us (or come to the IRC channel).

Contributor

reaperrr commented Nov 21, 2017

First of all, I agree with the general fix; the idea behind the aim sequences wasn't bad, but with only 8 facings, the glitching always hurt more than the raised rocket/ramp added. This should've been either upgraded to a full 32-facing sprite, or removed again long ago.

If we really wanna remove aiming graphics a better solution would be use Ammo, Conditions and 2 WithSpriteBody s and get rid of WithAttackAnimation imo.

This would be the best solution, yes.

But regardless of which approach, the "aim" and "aim-empty" sequences should go away now instead of becoming duplicates, WithAttackAnimation can use idle(-empty) as well.

@Arular101 If you need help with setting up the AmmoPool approach, notify us (or come to the IRC channel).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 21, 2017

Member

This is a very visible and well known bug, so it would be silly to not fix this as part of the next release.

Member

pchote commented Nov 21, 2017

This is a very visible and well known bug, so it would be silly to not fix this as part of the next release.

@pchote pchote added this to the Next release milestone Nov 21, 2017

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 22, 2017

Contributor

I was a bit too excited when I fixed that bug, that I completely missed the aim and empty-aim sequences were called in the rules/vehicles.yaml. I also want to fix this properly.

I did a little investigation of how to do it with the things MustaphaTR mentioned. I could implement the AmmoPool (with AmmoCondition: ammo) and ReloadAmmoPool. Then the RequiresCondition: !ammo could be used for the trait that replaces WithAttackAnimation. But, I think there is no trait that can be used for that. A new trait called something like WithReloadingFacingSpriteBody should be made, if I understand this correctly. I hope that this is something relatively easy to do because I don’t have much experience with C#. But at least I want to make an attempt to fix this properly. So, this weekend I think that I can come to the IRC channel.

Contributor

Arular101 commented Nov 22, 2017

I was a bit too excited when I fixed that bug, that I completely missed the aim and empty-aim sequences were called in the rules/vehicles.yaml. I also want to fix this properly.

I did a little investigation of how to do it with the things MustaphaTR mentioned. I could implement the AmmoPool (with AmmoCondition: ammo) and ReloadAmmoPool. Then the RequiresCondition: !ammo could be used for the trait that replaces WithAttackAnimation. But, I think there is no trait that can be used for that. A new trait called something like WithReloadingFacingSpriteBody should be made, if I understand this correctly. I hope that this is something relatively easy to do because I don’t have much experience with C#. But at least I want to make an attempt to fix this properly. So, this weekend I think that I can come to the IRC channel.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 22, 2017

Contributor

@Arular101 That shouldn't be necessary. You just need one WithFacingSpriteBody@LOADED with RequiresCondition: ammo (and Sequence: idle, but that's the internal default so you don't need to add that), and add another WithFacingSpriteBody@EMPTY with RequiresCondition: !ammo and Sequence: idle-empty, unless I missed something, that should just work.

Contributor

reaperrr commented Nov 22, 2017

@Arular101 That shouldn't be necessary. You just need one WithFacingSpriteBody@LOADED with RequiresCondition: ammo (and Sequence: idle, but that's the internal default so you don't need to add that), and add another WithFacingSpriteBody@EMPTY with RequiresCondition: !ammo and Sequence: idle-empty, unless I missed something, that should just work.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 23, 2017

Contributor

Thanks for the help guys! It now works with ammo condition!

I was just wondering where these "@..." can be found. Because I searched for that but couldn't find anything about that.

Contributor

Arular101 commented Nov 23, 2017

Thanks for the help guys! It now works with ammo condition!

I was just wondering where these "@..." can be found. Because I searched for that but couldn't find anything about that.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 23, 2017

Contributor

Consider the @suffixes as UIDs - they aren't derived from classes at all and only there to differ between trait instances in yaml.

Contributor

GraionDilach commented Nov 23, 2017

Consider the @suffixes as UIDs - they aren't derived from classes at all and only there to differ between trait instances in yaml.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 23, 2017

Contributor

Ah thanks for the clarification!

By the way, before I forget it again, should the 8 facings from the aim and empty-aim be removed somewhere else too? Because we don't use them anymore.

Contributor

Arular101 commented Nov 23, 2017

Ah thanks for the clarification!

By the way, before I forget it again, should the 8 facings from the aim and empty-aim be removed somewhere else too? Because we don't use them anymore.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 25, 2017

Member

You've removed their sequences, which is the one and only place they are defined.

Member

pchote commented Nov 25, 2017

You've removed their sequences, which is the one and only place they are defined.

PipCount: 0
AmmoCondition: ammo
ReloadAmmoPool:
Delay: 240

This comment has been minimized.

@pchote

pchote Nov 25, 2017

Member

It might be a good idea to reduce the weapon reload delay (which is now unused) to 0 to avoid future bugs or confusion when the two get out of sync.

@pchote

pchote Nov 25, 2017

Member

It might be a good idea to reduce the weapon reload delay (which is now unused) to 0 to avoid future bugs or confusion when the two get out of sync.

This comment has been minimized.

@pchote

pchote Nov 25, 2017

Member

But you will also need to add a PauseOnCondition: !ammo to prevent the weapon from firing when there is no ammo!

@pchote

pchote Nov 25, 2017

Member

But you will also need to add a PauseOnCondition: !ammo to prevent the weapon from firing when there is no ammo!

@pchote

👍 after that minor suggestion.

PipCount: 0
AmmoCondition: ammo
ReloadAmmoPool:
Delay: 240

This comment has been minimized.

@pchote

pchote Nov 25, 2017

Member

But you will also need to add a PauseOnCondition: !ammo to prevent the weapon from firing when there is no ammo!

@pchote

pchote Nov 25, 2017

Member

But you will also need to add a PauseOnCondition: !ammo to prevent the weapon from firing when there is no ammo!

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 25, 2017

Contributor

Updated. Changed the weapons ReloadDelay to 0, and added the PauseOnCondition: !ammo. This should indeed avoid confusions in the future.

Contributor

Arular101 commented Nov 25, 2017

Updated. Changed the weapons ReloadDelay to 0, and added the PauseOnCondition: !ammo. This should indeed avoid confusions in the future.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 25, 2017

Contributor

I have to check the V2 rocket launcher used in Fort Lonestar.

Contributor

Arular101 commented Nov 25, 2017

I have to check the V2 rocket launcher used in Fort Lonestar.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 25, 2017

Member

Fort Lonestar overrides the reload time on SCUD to 280 ticks. That should be moved to V2RL instead. It is the only official map that customizes the V2 weapon.

Member

pchote commented Nov 25, 2017

Fort Lonestar overrides the reload time on SCUD to 280 ticks. That should be moved to V2RL instead. It is the only official map that customizes the V2 weapon.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 25, 2017

Contributor

Updated. Now this also works with the V2 rocket launcher in Fort Lonestar.

Contributor

Arular101 commented Nov 25, 2017

Updated. Now this also works with the V2 rocket launcher in Fort Lonestar.

@reaperrr

Disable or override inherited WithFacingSpriteBody:

Show outdated Hide outdated mods/ra/rules/vehicles.yaml
@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 26, 2017

Contributor

Updated. MustaphaTR mentioned that as well but I did something wrong back then apparently. It now works.

Contributor

Arular101 commented Nov 26, 2017

Updated. MustaphaTR mentioned that as well but I did something wrong back then apparently. It now works.

Fix V2 Rocket Launcher facings
Now with AmmoPool

V2 changes for Fort Lonestar

@reaperrr reaperrr merged commit 3ed18f5 into OpenRA:bleed Nov 26, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Nov 26, 2017

@Arular101 Arular101 deleted the Arular101:v2fix branch Nov 26, 2017

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 28, 2017

Contributor

@reaperrr, @pchote, sorry to bother you guys again, but currently at bleed the V2 rocket launcher is invisible and unselectable.
1
I had this error previously too when it was suggested to change for the first time. But when it was suggested to change for the second time, I probably tested the wrong file, and didn't catch this error. These two ways don't work:
WithFacingSpriteBody:
WithFacingSpriteBody@EMPTY:

-WithFacingSpriteBody:
WithFacingSpriteBody@LOADED:
WithFacingSpriteBody@EMPTY:

Only way I was able to get it work was with this:
WithFacingSpriteBody@LOADED:
WithFacingSpriteBody@EMPTY:

How should I proceed?

Contributor

Arular101 commented Nov 28, 2017

@reaperrr, @pchote, sorry to bother you guys again, but currently at bleed the V2 rocket launcher is invisible and unselectable.
1
I had this error previously too when it was suggested to change for the first time. But when it was suggested to change for the second time, I probably tested the wrong file, and didn't catch this error. These two ways don't work:
WithFacingSpriteBody:
WithFacingSpriteBody@EMPTY:

-WithFacingSpriteBody:
WithFacingSpriteBody@LOADED:
WithFacingSpriteBody@EMPTY:

Only way I was able to get it work was with this:
WithFacingSpriteBody@LOADED:
WithFacingSpriteBody@EMPTY:

How should I proceed?

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Nov 28, 2017

Member

Looks a bit like #14398.

Member

abcdefg30 commented Nov 28, 2017

Looks a bit like #14398.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 28, 2017

Contributor

@Arular101 it's not your fault, it's a code bug on bleed that we'll hopefully have debugged and fixed quickly.

Contributor

reaperrr commented Nov 28, 2017

@Arular101 it's not your fault, it's a code bug on bleed that we'll hopefully have debugged and fixed quickly.

@Arular101

This comment has been minimized.

Show comment
Hide comment
@Arular101

Arular101 Nov 29, 2017

Contributor

Ah, okay, good that it is known.

Contributor

Arular101 commented Nov 29, 2017

Ah, okay, good that it is known.

@cjshmyr cjshmyr referenced this pull request Jan 1, 2018

Open

TODO List #1

40 of 77 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment