DC37 / Super-Mario-Paint

Super Mario Paint - A remake of the music composer in Mario Paint (1992)
MIT License
162 stars 24 forks source link

Crashes too often/won't load arrangement files #30

Closed SuperNintendoGameboy closed 6 years ago

SuperNintendoGameboy commented 7 years ago

Not really sure if this only applies to the Mac OS version, but sometimes whenever I add a lot of layers, the program eventually crashes and I lose any progress so I have to save like every 2 minutes or so. Even if there isn't a lot of notes on screen at once, this will occasionally happen regardless. I also have arrangement files saved, but the arranger section won't load them for some odd reason.

DC37 commented 7 years ago

I have definitely run into this issue, it's likely not a single issue but if you know how to debug Java and threaded code, the help would be greatly appreciated. I've gotten it narrowed down to the event handler which puts all of the notes on the screen, and it seems like something is not thread-safe even though the API says it should be. I have been struggling with this for a good part of this year.

j574y923 commented 7 years ago

Been testing this... I have noticed that this error happens when there are a bunch of note layers and you move your mouse inside the staff area as you scroll. It doesn't crash for me in the following scenarios:

1) Scroll + mouse inside staff area but mouse not moving 2) Scroll + mouse outside staff area doing anything (as long as there is no instrument silhouette beneath your cursor you can't crash the program by scrolling).

Redrawing notes as you scroll AND having the staff pane listen to mouse events seems to cause the program to desync. I think more note layers makes it harder to keep up and will lose track of which measure line is getting focused by the moving mouse resulting in ArrayIndexOutOfBoundsException.

I am working on a temporary hack right now that will disable the staff pane from listening to events as it scrolls thus preventing this desync. Hopefully I can inspire some ideas and we can come up with a proper solution very soon!

DC37 commented 7 years ago

allthenotes.txt Here's a file that should induce the error more often. I think you're correct that the problem is happening when multiple things are accessing the same ObservableList at the same time, I also found some methods that would allow one to create ObservableLists that are thread-safe, but have not been able to play with said implementation much. @j574y923 I think your hack should fix the problem quite readily, and at least stabilize the program for proper usage.

j574y923 commented 7 years ago

How'd you respond so quick lol 😮

thanks, I'll take a look tomorrow. a bit late here. Here's the file I was using if you want to see mine... test3.txt

DC37 commented 7 years ago

I happened to be on my email and saw some action happening on the thread, decided to respond 👍 Agreed, quite late.. haha Working on merging a few other things. Work hours are weird for me.

j574y923 commented 7 years ago

Hey @DC37 , what other details can you tell us about this multithreading issue? Would like to hear a little more about it if you have any interesting tidbits... Also how would you suggest we approach this problem ?

Let me comment some stuff I've tried (don't know if I implemented 100% correctly)... Things I've tried but were unsuccessful: 1) wrapping observablelist with synchronizedobservablelist before adding notes into it 2) clearing all notes first in one loop then populating notes in a separate 3) sleeping thread between each note clear call and each note populate call 4) keyframe animation in timeline to populate note at different times on a separate thread

DC37 commented 7 years ago

@j574y923 unfortunately I think we are on the same page as to what you have tried already. I have also tried making a thread to do the population of the staff with notes, with a queue that is definitely on the main JavaFX application thread, but that didn't work either. It is possible that this is a bug with the API, not sure what we can do about the bug but work around it. I think your method of disabling scroll and updating after things are done might be the fastest way to get things running...

j574y923 commented 7 years ago

Ok I think I fixed the threading problem with my little hack now. It is in the pull requests and since testing the change I haven't had my Super Mario Paint crash on me.

(If I wasn't clear before, disabling means that the node will just ignore all mouse events but can still call the API)

What I did was I disabled all the stackpanes and then created a modified version of your StaffInstrumentEventHandler class. Disabling all those stackpanes means the StackPane ArrayList's listeners are ignored, StaffInstrumentEventHandler_Hack is added to the entire scene and listens for mouse events instead of having a bunch of StaffInstrumentEventHandlers all listening to events before.

Also added drag-adding notes and removing notes. =D

here's a video of the changes: https://streamable.com/j20rt

DC37 commented 7 years ago

Awesome!! I will get onto testing this when I can, probably in the next few days. If it's stable then we can clean up the code a bit and rebase/merge it into the master branch :) :+1: :+1: :+1: :heart:

j574y923 commented 7 years ago

Best part is that only 1 file is added and 1 file modified thanks to a well designed framework!

Btw can i ask what that allthenotes.txt song is? It was catchy

DC37 commented 7 years ago

I took one of Pokesonicddrninja's Chemical Plant Zone ( https://www.youtube.com/watch?v=Z5HP-mslB5s ) files and added a bunch of notes to it. It was one of the first files that I had that was causing errors on a regular basis, so I just added even more notes to it to try to get a "realistic" test. I was also running into those errors when doing Moon Revenge ( https://www.youtube.com/watch?v=f9mvXKhtSbY )

Happy to hear that the framework is well designed! I only code for fun / haven't really had much formal education in software engineering past the basics. (I do electrical and mechanical engineering type things normally)

j574y923 commented 7 years ago

Well done you can be compsci any day

DC37 commented 6 years ago

I think we resolved a good chunk of these issues with the recent commits.