FlxWeapon rework

[DalekCraft2]

Clarified the outdated doc comments, removed unused fields, and changed how fireRate is implemented to make it more accurate to the game's time (because the old implementation didn't properly respect the game pausing when tabbed out). Also made FlxWeapon destroyable.

If this gets approved, I would also like to know whether I should destroy the FlxSounds or simply set them to null. Regardless of whichever one is picked, I'll still be making a commit to remove that TODO comment in the destroy() function.

[Geokureli]

This has breaking changes on a niche, legacy class. At a glance, I see odd things I wouldn't recommend, like using a FlxTimer to control fire rate when I would use a float and add elapsed on update. I recommend making a new class in a new repo and battle-testing it in your game project over the course of it's development and then we can talk about replacing this entirely in a new major version.

FlxWeapon was ported from the flash version of flixel back when sh'mups were all the rage, and it isn't used very often, now. If I were making a shoot 'em up, I would probably just make my own system. Looking at the old version and you're version, they're pretty different from my version would look that, and that's fine, but it makes me thing we shouldn't even have an "official haxeflixel version" and just make little repos that people can copy

[Geokureli]

Can you list the changes made, and why they were made, @DalekCraft2 ?

[DalekCraft2]

Oh, if you are considering this again, give me a moment. I need to make a few more fixes.
I described the changes in the name of each commit, if you want to look at those.

[Geokureli]

I think its just confusing becuase of how the diff isnt aligned, well

[DalekCraft2]

You mean how some commits contain changes what were not mentioned in the commit's name?

[Geokureli]

I usually dont look at commits individually, just the whole diff ill take a closer look but i know i dont want to have a nother breaking change in flixel-addons this quickly, we had just released 3.0.0. I'd say make the non breaking changes first, and then consider a rewrite in the way i outlined above

[DalekCraft2]

I would have used update() for the fire timer, but the thing is that FlxWeapon does not implement IFlxBasic nor extend FlxBasic. If it was to have an update() function, it would probably be called by a manager, similarly to how FlxTween and FlxTimer work.

[DalekCraft2]

Here are the changes what I made and why I made them:

• Changed function arguments to start with lowercase letters because I'm pretty sure you're already trying to do that with all of Flixel
• Went through all of the doc comments and removed references to outdated functions because those won't be helping anyone
• Removed unused fields because they were likely replaced by speedMode and fireFrom at some point, though I could have also given them accessors what reference their respective fields in the enums
• Made FlxWeapon implement IFlxDestroyable because it has a few fields what can be put() or destroy()ed
• Changed the implementation of fireRate to use an FlxTimer because the old implementation did not respect how the game pauses when it is tabbed out
• Overrode FlxBullet#destroy() so it calls bounds = FlxDestroyUtil.put(bounds), to be safe
• Changed some FlxPoint-related things in some functions like runFire() and rotatePoints() to ensure that no FlxPoint would be taken from the pool but not returned

[SeiferTim]

I'm going to side with @Geokureli here and say that FlxWeapon is kind of an outlier and should probably be removed altogether.
It sort of implies that using this class is 'the right way' to do projectiles in your game, and, in reality, there are many, many different ways that might fit and individual project better than trying to use this class - especially since it breaks so many of the normal conventions that are staples in HF.
If we could get away with removing it, I would love to have maybe a Sample that shows some of the more modern techniques to doing things like this for people to base their code on rather than have a class that is a 'cookie cutter' solution for everyone.
Maybe even a section in Snippets dedicated to 'Game Mechanics' and have some samples of weapons in there.

Doing a little poking around in flixel-addons there are a couple of other classes grandfathered over from AS3 Flixel and Photostorm that I think might not be working properly anymore (FlxStarfield seems to be not working?)

[Geokureli]

no breaking changes can be introduced in this PR

[Geokureli]

I'm going to side with @Geokureli here and say that FlxWeapon is kind of an outlier and should probably be removed altogether. It sort of implies that using this class is 'the right way' to do projectiles in your game, and, in reality, there are many, many different ways that might fit and individual project better than trying to use this class - especially since it breaks so many of the normal conventions that are staples in HF.

Agree with all of this.

Doing a little poking around in flixel-addons there are a couple of other classes grandfathered over from AS3 Flixel and Photostorm that I think might not be working properly anymore (FlxStarfield seems to be not working?)

Got more examples?

[DalekCraft2]

I did not reimplement one of the variables, lastFired, because it was private and not used anywhere, so let me know if I should add that back.

[Geokureli]

@DalekCraft2 I made some changes

Passed in objects used as the return value are typically named result, I also simplified some lines in rotatePoints.

when editing doc comments use the following formatting:

• no tabs after the left "*" margin
• 3 spaces between @param and the param name
• with spaces, indent all param descriptions to be 2 space characters right of the longest param
• try to keep lines under ~120 characters, count tab indents as 4 characters (vscode should tell you the Col in the bottom right)
• After you've decided to split a long line try to keep the resulting lines under 100 columns (this does not mean that all lines must be under 100 columns!)
• when splitting param descriptions into multiple lines, indent the second line, with spaces so it starts at the same column as the first
• 2 spaces after @return
• no need to indent the second line of a return description, since there's only one return value
• I should add this to the formatting guide

here's an example, in my VSCode editor with "Render Whitespace" set to "boundary'

another example of param description indenting

[Geokureli]

removing this is technically a breaking change but i'll allow it. In all likelyhood no real person will have a compile error unless they use this field in an extending class, and highly unlikely and easy to fix.

In the future, when in doubt, just deprecate it

[Geokureli]

when making these changes did you have a project you used to test them out? If so, is it small and public? Can you share it?

I try to make a public test state for every feature, fix, or improvement I make to flixel, and use it to help others, or make unit tests. Here's an example: HaxeFlixel/flixel#2768

If you don't have a small one, I'll make my own, but it might take a bit as I'm busy with work atm

[DalekCraft2]

I had originally made the FlxTimer/IFlxDestroyable code in Quench and then decided to try to make a PR for them, so I knew that those worked already. I agree that it would be better to do a less complicated test, though.

Also, it would probably be a bad idea for you to test it with Quench because I use my awful Git habits over there constantly. By this point, every Haxe file in the VS Code directory viewer has a yellow name, meaning the version what you would see on the GitHub page would be quite outdated.

[DalekCraft2]

So what is the consensus for this PR?

[Geokureli]

I still need to test the changes and I haven't had time because FlxWeapon is pretty low on my list of priorities. My opinion of FlxWeapon is that it probably shouldn't exist and some new standalone tool should be made from the ground up, however I don't want to remove it until the next major version release, which isn't any time soon so I'm allowing this change if it doesn't have any problems.

[Geokureli]

I finally tested this. Lot of issues.

1. I get deprecation warnings if my project uses FlxWeapon, because FlxWeapon itself uses these deprecated fields.
2. I get runtime errors for what seems like a basic implementation
3. I find FlxWeapon to be incredibly confusing, it's possible I'm doing something wrong but it's so cumbersome to even set up a basic weapon that fires in the direction the parent is facing

Here's in my example, so far:

    var weapon:FlxWeapon;
    var gun:FlxSprite;
    
    override function create()
    {
        super.create();
        
        add(gun = new FlxSprite(100, 100, "assets/images/haxe.png"));
        weapon = new FlxWeapon
            ("test", (w)->new FlxBullet()
            , PARENT
                ( gun
                , new FlxBounds(FlxPoint.get(0, 0), FlxPoint.get(gun.frameWidth, gun.frameHeight))
                , true
                )
            , SPEED(new FlxBounds(5.0, 10.0))
            );
    }

I get the sense these changes weren't tested, the error happens because set_fireFrom sets useParentDirection which calls set_useParentDirection which crashes because it's checking fireFrom, which hasn't been set yet. I tried looking through quench, but there's nothing to grab thats easy to duplicate for testing.

Truthfully, after have a taste of FlxWeapon I leaning towards retiring this feature. I think it's poorly executed and never have been in flixel to begin with. I also hate FlxBounds and want to remove all references to that. I'm even considering removing the tool in the next major version, I would create a new repo called LegacyFlxWeapon that people can use if they have old projects that need it, either that or just deprecate the entire class or put a huge disclaimer on it to prevent people from creating issues or PRs to this. It's just too niche, and it's not worth my time compared to just about any other flixel feature

I think you can make a far better weapon/bullet manager than this, and you absolutely should, and we'll add it to the external tools list.

[Geokureli]

If i were to try and save this class, I would do something like this, but I still think the class is trash and should just be rewritten in a new repo

[DalekCraft2]

That's fair. Why do you dislike FlxBounds, though?

[Geokureli]

That's fair. Why do you dislike FlxBounds, though?

Its mainly used to define rects via literals but FlxRect would allow that more compactly. FlxRect has more features than FlxBounds, too. this is basically the only thing that uses it