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

ALUtils.java: Delay logged message 'building' #105

Closed acodili-jg closed 1 year ago

acodili-jg commented 1 year ago

TLDR;

This update introduces checkError(Supplier<String>) for lazily built log messages.


The Issue:

In the current code, messages passed to checkError(String) are, potentially complex and eagerly built, string. However, this approach may unnecessarily create messages that may never be used due to the way checkError(Consumer<String>) works.

The Proposed Solution:

checkError(Supplier<String>) tries to address the issue by providing a way to defer the message building when it's needed, saving unnecessary processing when no errors occur.

Alternative Approach:

As an alternative, the underlying logger's functionality can be used directly. One interpretation could be to discourage the use of checkError(String) in favor of checkError(Consumer<String>) and do the logging there, though it may result in duplicated code.


I would really welcome feedback as I don't understand why I'm so hesitant in making this pull request.


Transparency

ChatGPT helped in rewriting the final description.

Original description. > ### TLDR; > > `checkErrors(Supplier)`'s allow message strings, usually built before being sent to `checkErrors(String)`, to be made or built when needed lazily. > >
> > `checkErrors(Consumer` will only process or consume an error message if debug logging is enabled and when there is an error to work with. `checkErrors(String)` internally calls `checkErrors(Consumer)` with a consumer that consumes the parameter, the error message, as a message logged by the `Engine.LOGGER`. I propose an improvement to `checkErrors(String)`, or rather an alternative is the new method `checkError(Supplier) as we can provide it the how to build the message string we want, albeit if immutable data are used, and it will only perform the message building when a message is needed. > > Alternative: expose the logger part to allow the utilization of its functionality that otherwise was simplified as a single string.
ChatGPT's suggestion. > ### TLDR; > > This update introduces improvements to how errors are handled and logged in the `ALUtils` class. > >
> > #### The Issue: > > In the current code, error messages are logged using a fixed string format. However, this approach has some limitations, as it may log unnecessary messages or build complex error strings even when there are no errors. > > #### The Proposed Solution: > > To address these limitations, we are introducing a new and more flexible method called `checkError(Supplier)`. This method allows us to provide a "blueprint" for building the error message only when it's needed, saving unnecessary processing when no errors occur. > > #### What's Changed: > > 1. We added three overloaded `checkErrors` methods: > - `checkErrors(Consumer callback)`: This method takes a function that can consume the error message. It will only process and log the message if debug logging is enabled and when there's an actual error to report. > - `checkErrors(String message)`: This method calls the above method internally, but it now accepts a direct error message string. This is kept for backward compatibility. > - `checkErrors(Supplier messageSupplier)`: This method allows for lazy evaluation of the error message. It takes a supplier function that provides the error message when requested. > > 2. We also introduced new methods for handling specific types of errors, such as `errorApply`, `errorProperty`, and `errorSet`. These methods utilize the `checkErrors` functionality for consistent error handling. > > #### Why It Matters: > > By using the new `checkError(Supplier)` method and the improved error handling methods, we can build error messages more efficiently and avoid unnecessary overhead. Additionally, this enhancement gives developers more control over error logging and helps improve the overall code quality. > > #### Alternative Approach: > > As an alternative, we considered exposing the underlying logger's functionality directly, but we believe that the new `checkError` method provides a cleaner and more maintainable solution, keeping the logger implementation encapsulated. > > We welcome feedback and reviews to ensure that these changes are beneficial and align with the project's goals.
mikenrafter commented 1 year ago

For future reference (don't do anything about this now) (and it's not really your fault), but this PR would probably be better suited on its original branch. I've been meaning to solidify the workflow around here for some time now. (adding that to my TODOs rq)

On another note, looks reasonable.

acodili-jg commented 1 year ago

I guess #104 wasn't a mistake; and sure, I'll check Discord.

mikenrafter commented 1 year ago

Could you integrate this into relevant places in the code? As well as make a variant of this available for non-error logging? Computational offload inside Engine.java for logging is something I cheaply implemented before. However, a more structured approach like this one would do it some good.

acodili-jg commented 1 year ago

Sure. Though I'll be closing this one (later) to make a new PR as this PR's branch is actually based on master and it seems there is newer code on octree-testing that I might miss.