thedocruby / resounding

A New Minecraft mod that provides realistic audio physics using parallel wave tracing and an improved physics algorithm.
https://thedocruby.dev/resounding
GNU Lesser General Public License v3.0
84 stars 4 forks source link

Memoization: exception handling #111

Closed acodili-jg closed 4 months ago

acodili-jg commented 4 months ago

What changed?

What didn't change (yet)?

I might have missed something, but hopefully this is all the main points.

mikenrafter commented 4 months ago

I think a lot of what's going on here has some merit, however there are a few things that need modification here:

  1. Though Java functions in the wild tend to be extremely verbosely named, we shouldn't continue the trend. Things like MemoiseGetAllDependenciesAnyways are far too verbose, and are a bit hard to grasp what's going on with.
  2. Some comments & Javadoc have some invalid grammar—this' minor.
  3. Don't deprecate classes and methods the project isn't going to use anymore. We have the git history if we want to view them, and this isn't a library with public releases (yet?) so there's no need to have any sort of backwards compatibility.
  4. I think this implementation is a bit clunky. Now, Java often feels this way to me, so perhaps it is the best solution, and I'm wrong here. However, I'd like to give it a go with implementing these ideas in a slightly simpler manner.

Before bothering with these changes, let me give an implementation a try. I'll post updates here.

acodili-jg commented 4 months ago

I agree with the points mentioned and it's good to know yours and hopefully others' perspectives. Regarding the deprecation, mostly on Utils.memoize, I was using it to get a warning on usage of it, though thinking back I could have used TODO comments.

mikenrafter commented 4 months ago

Ended up going with my implementation (as discussed on discord). See a182489 for details.