Closed greysondn closed 10 years ago
Dovyski
commented
10 years ago The code looks good! I just don't like those updateGUI() and updateGUIFromMouse methods. They seem a bit confused to me. Can't we replace them with refresh() or update()?
greysondn
commented
10 years ago Summary of points to follow:
updateGUI() and updateGUIFromMouse() do what they say precisely; they are not update() as we generally think of in Flixel (see above) and definitely renaming updateGUI() seems like a bad idea. Names that work well for one class to describe the closest thing to an update() don't work for the other.VCR and FlxGame; make these (well, actually, FlxPanel) into proper FlxGroups so they they can benefit from the standard update() cycle and adjust them accordingly.Can I be frank for a few minutes and address how poorly architected this code really is to me?
Let's establish a frame of reference for this discussion; the current Master for Vis and VCR will do fine. (should Master change and those aren't permalinks, I refer to v2.56)
So, I noted in my original submission that
the logic here is really a fudge that breaks ... object orientation; buttons should be responsible for themselves
In Vis at 154 is UpdateGUI(); it matches its namesake. Based upon buttons hovered and pressed it handles the GUI. A similar function exists in VCR at 564.
These two functions read class member variables, decide where the mouse must be in relation to the toolbar, and handle changing the visuals accordingly by reaching straight into the button's variables and adjusting them.
When I saw this, I grew a bit concerned by the nature of these two classes and wondered why this was necessary. The answer is straightforward enough - there's no class defined for the buttons to leverage and we're not extending FlxBasic so there's no standard update procedure in game logic.
However, I don't know if that's viable. That work was out of my time constraints then and - of these two patches -this is the "simpler" and "less important".
What I realized was that I could trivially to extend FlxGroup for the FlxPanel if I wanted - code reorganization needn't happen overnight, after all - but replacing them with FlxButton was insufficient - we needed on hover and a few other tricks. I started to draft the class and then realized I was only repeating work we'd not gotten to a sold decision on last I'd heard:
Are we merging in Flixel Power Tools? ... FlxButton from the Power Tools kit would be - in effect - what I'd end up writing
...
Being said:
I don't know that the right name is update() for updateGUI() and naming it such may cloud the conversation later if we go to convert this to an FlxGroup later. Certainly it does what it says on the tin right now. From a "this is straightforward" perspective, I see no reason to change it; the name correctly describes what the function is doing and not all aspects of what we'd call an update() in flixel happen inside of it. I would offer similar reservations at this time for most names offered for it, including refresh() as there are other updates to the state and no really hard resets in both of these classes outside of it (meaning there's no general refresh for the entire class's state). A refreshGUI() is merely throwing a thesaurus at the desire to change something.
...
There is, aggravatingly, no single point of attack to resolve this into having just one function that handles all updates. The nearest one would be a consolidation of a common scrap of code in Vis:
unpress();
checkOver();
updateGUI();This became updateGUIFromMouse(). The name is painfully long - and I hate it, too - but this is accurate. Each of those functions does what it says: unpress() all buttons and reset states, check what the mouse is over (checkover()), and then updateGUI(). Naming that update() may be not too bad or too cloudy later; naming it refresh() I wouldn't argue too strongly with, at least inside of Vis. (There is also the issue that it's manually fired, whereas update() happens - ideally - on a regular interval.)
...
This still would leave VCR. While that scrap does appear in VCR, the impression I'm under is that most work that would fire updates to portions of state in the class are handled by FlxGame (which I've not even opened yet), meaning that there is no "sane" thing to call update() in VCR - and that more violation of OOP has occurred where VCR should be mostly responsible for itself and does not appear to be. Future work should include ascertaining how to disentangle this mess into once nice update() function if at all possible.
So the long and short is that any rename that would apply to Vis wouldn't necessarily apply equally well to VCR.
...
Whatever you decide on this matter, if you want to take the leap of authority, is what we'll go with. The best I can do - have tried to do, always, here - is provide what I know and an excess of notes that may be at least somewhat useful. Here, the nuances are merely those of idealized practice at best.
Dovyski
commented
10 years ago You convinced me to keep the names updateGUI() and updateGUIFromMouse(), @greysondn :) As you described, the code is extremely entangled and renaming methods now won't help.
I'll make a few tests and merge this soon. In the future we should discuss about using Flixel own button structure (FlxButton and the state's update list) for debug overlays instead of Flash Sprites. Right now all debug code is using "pure" Flash classes, which is causing all that update/OOP confusion.
Dovyski
commented
10 years ago I've inspected the code. Excluding that extra return at line 497 in VCR, the code is good to go. As soon as you fix the problem, I will merge this pull request.
greysondn
commented
10 years ago This will be done very soon. I'm sorry I'm behind a bit; I did keep up for a short while after explaining myself, but I missed when you finally replied or I could have it done in a few minutes.
I will get to this very, very soon.
greysondn
commented
10 years ago Writing code without coffee may result in silly errors ;)
To the curious onlooker, there were commits I forced out of the record. Originally, I had removed the entire line (which is incorrect) and pushed it with the wrong date and a simple comment.
Hm. My system clock is incorrect on the computer used to do the fix. Beyond this oddity, it is done.
Post-coffee, I looked it over and saw the error so I posted a third commit to fix the second, with the comment
Writing code without coffee may result in silly errors ;)
It took maybe ten minutes to remember that flixel maintainers prefer a clean record so I posted a hard reset (the lack of reaction and time of day told me it was unlikely anyone had read it over yet) to the state of the first commit, then simply redid the second commit correctly.
Like a certain game show, the current second commit should be my final answer :laughing: (For bonus points, I even fixed my system calendar on the relevant machine.)
Dovyski
commented
10 years ago Awesome! :D Thanks for all the work!
Don't worry about the response time, we all develop for Flixel Community during our free time. No need to rush.
I would suggest seeing #160 for an excess of discussion about roughly the same work.
Recommendations moving forwards...
Are we merging in Flixel Power Tools? (I suggested this in #163) Most of the logic here is really a fudge that breaks what I feel is proper object orientation; buttons should be responsible for themselves and FlxButton from the Power Tools kit would be - in effect - what I'd end up writing if I'd extract them to do as such within the scope of how Flixel works. I believe @IQAndreas had hinted at similar work (and a few ideas were tossed around besides) in #160's conversation.
Beyond that the new class is semi-abstract. I've sort of cherry picked what I feel is correct for a parent (or, rather, not incorrect) and this results in a couple of functions that exist purely as placeholders (their bodies will have a "pass" comment).
If there's any notes on this work or anything that I could do let me know. I have done preliminary testing and everything seems sound - however, it never hurts to do extra testing, especially for something so important :)