petertalbanese / SoundSwapper

Allows the user to replace sounds with custom .wav files.
BSD 2-Clause "Simplified" License
1 stars 4 forks source link

peformance improvement and cleanup #3

Closed Zoltus closed 1 year ago

Zoltus commented 1 year ago

The new update was great but it still had the problem where it searches from list with a size of 10k if the sound is changed. I changed the soundlist to Arraylist and it only contains ids which will be replaced, if u replace 10 sounds it will find it way faster and the code implementation is also simpler.

on the updateList() it clears the list and tries to parseint, if it fails it will log warning. else it will add the soundid from the config to the list.

You had some isInteger but parseint already does all that. and trycatch will catch if its not integer.

new BufferedInputStream and new FileInputStream use finalize() method which is deprecated and can cause problems and be a bit slower but in this scale it doesnt really matter but just for good practice i made it use methods in Files class. And Notifier.class.getResourceAsStream(sound_name) already returns inputstream so no need do do new new bufferedinputstream from inputstream.

And you had Integer.toString which i removed also since java calls tostring method automaticly to everything if its put inside string

I really hope you merge these changes. I havent tested them since i have no idea how to compile/use runeliteplugins.

Thanks for listening.

petertalbanese commented 1 year ago

Really appreciate the help! I'll proper review and test this after work today, but just some quick thoughts based on your comment:

Definitely agree on the long list being unnecessary. Would it be better to use a HashMap over an ArrayList if we're building it up like this though? This would maintain the O(1) lookup time of the long list, but still fix the poor memory design.

I used isInteger over try/catch intentionally since it is more efficient than exception checking parseInt. I'm fine with this change though as I think it looks cleaner and the difference is minimal.

Also thanks for the input stream stuff, I didn't really have much experience with that so this sounds like an improvement.

Zoltus commented 1 year ago

Really appreciate the help! I'll proper review and test this after work today, but just some quick thoughts based on your comment:

Definitely agree on the long list being unnecessary. Would it be better to use a HashMap over an ArrayList if we're building it up like this though? This would maintain the O(1) lookup time of the long list, but still fix the poor memory design.

I used isInteger over try/catch intentionally since it is more efficient than exception checking parseInt. I'm fine with this change though as I think it looks cleaner and the difference is minimal.

Also thanks for the input stream stuff, I didn't really have much experience with that so this sounds like an improvement.

I mean hashmap could be used if you want to have custom names for the audio files other than id. But if u just name the sound with the id arraylist is better for that. Also for the inputstream stuff, tbh idk much about that too but I had this intellij plugin warning me about it and then I googled the reasons:D

Actually i think HashSet could be better because we dont want the list having duplicate values, It doesnt have but anyways incase there was a bug and it is O(1) (Had to google what O(1) means:)) U learn smthing every day

Zoltus commented 1 year ago

The current code with the arraylist with booleans also lags insanely you can test this at neitiznot trolls for example. just put any sound file for these sounds 518,849,860,871,1980,2399,65535,2569,2567,866 and go there and you will notice the freezing every second. So i hope the approach above fixes it:) but will see unless the problem is on the way the sounds are played

Can you tell me how i could test the plugin my self with my own edits? 2022-11-12_15-32-57

Zoltus commented 1 year ago

So i think the problem isnt the maps/lists the problem is that every time sounds need so be played it calls the tryload I think it would be better to store the clips instead of making new ones. Atm I think the clip gets removed on gc And mayby would be better to keep them in a map with the sound id. Every time clip is loaded it freezes. Other option would be to load clip async but we want the sound so be syncronous.

petertalbanese commented 1 year ago

Hey sorry I was away for the weekend. Just sent you a request on Discord!

Here is the setup guide for running RuneLite from source: https://github.com/runelite/runelite/wiki/Building-with-IntelliJ-IDEA Here is the guide for creating new plugins for the hub: https://github.com/runelite/plugin-hub

I use a combo of these to test. I run RuneLite from source, and drop my plugin code into the plugins folder before building. You usually have to modify one of the includes in your files to make it work in the different file structure.