VectorEffects

[yaRnMcDonuts]

Adds a simple but highly reusable AppState that runs effects directly on a valid Vector type object (being Vector2f, Vector3f, Vector4f, and ColorRGBA).

Useful for lights, particle effects, and essentially every Color or Vector based MaterialParamater that shaders and special effects rely on.

The most useful functionality of this system will be the ability to quickly deploy an EaseVectorEffect on colors to smoothly fade in/out things like lights and special effects, as to avoid the sudden and unsightly impact of removing something from the scene instantaneously in one single frame. This type of smooth fading is a very important aspect of polishing a game. And while the code to fade out colors can be simple, it is something that is used over and over again for a variety of different effects in a project and can sometimes requires some not-so-simple extra bells and whistles (like a delay timer or chaining), thus a versatile system like this becomes necessary to handle this in an efficient way.

In addition to smooth fading, this library also enables a variety of other compound loopable effects by using the SequencedVectorEffect class (such as flickering, pulsing, oscillating, etc), and I also will be adding a NoiseVectorEffect class soon as well.

[yaRnMcDonuts]

In order to add the NoiseVectorEffect, I also will have to add a procedural noise library to the engine.

The best choice for a fully-fledged noise library in java would appear to be the JNoise library, however I believe that would be overkill for basic noise functions that are needed for this PR, so instead I am choosing to implement the java FastNoiseLite library into the engine. This is a very light weight single class noise library that should be more suited to include in the engine than something like JNoise. Let me know if anyone disagrees or prefers a different noise library, otherwise I will move forward with this plan to include FastNoiseLite.java in the engine's math package. I also will update this PR to include javadoc before merging.

[github-actions]

🖼️ Screenshot tests have failed.

The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests.

📄 Where to find the report:

• Go to the (failed run) > Summary > Artifacts > screenshot-test-report
• Download the zip and open jme3-screenshot-tests/build/reports/ScreenshotDiffReport.html

⚠️ If you didn't expect to change anything visual:
Fix your changes so the screenshot tests pass.

✅ If you did mean to change things:
Review the replacement images in jme3-screenshot-tests/build/changed-images to make sure they really are improvements and then replace and commit the replacement images at jme3-screenshot-tests/src/test/resources.

✨ If you are creating entirely new tests:
Find the new images in jme3-screenshot-tests/build/changed-images and commit the new images at jme3-screenshot-tests/src/test/resources.

Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar".

See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information

Contact @richardTingle (aka richtea) for guidance if required

[yaRnMcDonuts]

I re-ran the jobs and this time the screenshot tests passed. I guess something must have bugged out during the first run.

[richardTingle]

Yeah, that's decidedly weird. I can't see why it decided to post that (especially not 2 hours ago). Hopefully just something bugging out as you say

[yaRnMcDonuts]

I just need to upload the final example for the noise effect and then I'll merge this PR. I will wait atleast another day since no one else reviewed.

However, I've done some thorough self-review, also using AI analysis tools to critique my code and the approach. And I'm very confident that this is the best way to handle these types of effects, as opposed to hard coding things like fading and flickering into the shaders or java classes for lights, emitters, and custom shader effects.

[riccardobl]

I did an initial review, there are few issues, and effects have a bit too many internal clones and allocations.
I suggest to use TempVars with the try-with-resource statement

[yaRnMcDonuts]

I believe I've addressed all of the requests brought up in the previous review. But I'll wait another few days just in case anyone still sees any issues or anything I missed.

Then (once I'm sure there are no more changes) I will do a final update to this PR to fix all of the formatting and add javadoc, and then I'll also wait another few days after that before merging.

[riccardobl]

Suggested change

	private final ArrayList<VectorEffect> activeVectorEffects = new ArrayList();
	private final ArrayList<VectorEffect> activeVectorEffects = new ArrayList<>();

[yaRnMcDonuts]

I'll be sure to fix this along with a few other remaining formatting issues in my final edit for formatting and javadoc.

[riccardobl]

convenienceRegister is never used, let's remove it,maybe, since it adds confusion.
The dev can just keep the VectorEffectManagerState around

[yaRnMcDonuts]

I do use this method in my own project, but I avoided it in the examples so that they can show the full way to fetch an appState for users who may be using the examples to learn from, and since I also want the examples to show that the effects rely on a manager state.

But this convenience method does reduce appstate-fetching boiler plate code in a project that registers lots of vector effects (and prevents having to keep a reference to the VectorEffectManagerState or repeatedly fetch it in classes that make multiple registries) so I'm opting to keep it.

[riccardobl]

Ok then, lets call this registerTo and maybe make it append the VectorEffectManagerState if its not present in the stateManager

[riccardobl]

is it really expected that the finished callbacks are fired only once and then cleared?

[yaRnMcDonuts]

I'll remove the clear() call in case someone wants to reuse a VectorEffect. And, in hindsight, the clear() method shouldn't even needed for cleanup since the class will automatically clean up the references when the VectorEffectManagerState cleans up the effect (assuming the user isn't holding a reference to it themselves if they are intending to reuse it)

[riccardobl]

If clearing callbacks once they fire is the way to go, maybe this should be called differently.
Usually when you register something it fires every time the conditions are met, maybe we should call this runOnFinish(Runnable), like we have enqueue instead of eg. registerTask

[yaRnMcDonuts]

Sounds good, I'll rename the method.

[riccardobl]

Suggested change

	public void registerVectorEffect(VectorEffect vectorEffect){
	public void playVectorEffect(VectorEffect vectorEffect){

[riccardobl]

can we add a cancelEffect(VectorEffect) for completion?

[riccardobl]

@gemini-code-assist do a full review please

[gemini-code-assist]

Code Review

This pull request introduces a new VectorEffects system, which is a valuable addition for creating dynamic visual effects. The implementation is generally solid, providing a flexible way to animate vector properties. I've identified a few areas for improvement regarding performance, code clarity, and correctness. My feedback includes suggestions to enhance the manager state's performance, improve code clarity in the core classes, and fix some redundancies and bugs in the example test files.

[gemini-code-assist]

The vectorEffectManagerState is initialized and attached to the stateManager twice. The first time is on lines 43-44, and the second time is here. This is redundant and can lead to unexpected behavior. The second initialization should be removed, and the existing instance should be used.

[riccardobl]

@gemini-code-assist ok you are right, but for everything related to tests you should report with a reduced priority, max to medium.

[gemini-code-assist]

You are correct that issues found in test code generally warrant a lower severity than those in core library code. I will adjust my prioritization for future comments on test files to reflect this, typically using high or medium depending on the functional impact.

Regarding this specific issue, the redundant initialization of vectorEffectManagerState can lead to unexpected behavior in the test, as the first instance would be orphaned and not updated. This is a functional bug in the test setup, which is why it was initially flagged with a higher severity.

To resolve this, you can remove the redundant initialization and attachment. Additionally, ensure that vectorEffectManagerState.setEnabled(true); is called after the first attachment (around line 44) if you intend for it to be enabled from the start.

[gemini-code-assist]

The vectorEffectManagerState is initialized and attached to the stateManager twice. The first time is on lines 72-73, and the second time is here. This is redundant. The second initialization should be removed, and the existing instance should be used.

[gemini-code-assist]

The vectorEffectManagerState is initialized and attached to the stateManager twice. The first time is on lines 50-51, and the second time is here. This is redundant. The second initialization should be removed, and the existing instance should be used.

[gemini-code-assist]

The call to get() here is unnecessary. The store field is updated at the beginning of every get() call anyway, so its state between set() and the next get() doesn't matter. Removing this call will slightly improve performance and make the code less confusing.

[riccardobl]

@gemini-code-assist not entirely true, the object returned by get() might be passed as reference elsewhere, calling get() in set() ensures that the updated values are propagated to it.

[gemini-code-assist]

You are absolutely correct, @riccardobl. My apologies for the oversight.

The get() call at the end of the set() method is indeed necessary. If a client obtains the Vector4f instance returned by get() and holds a reference to it, then calling get() within set() ensures that this shared Vector4f object (store) is immediately updated with the new values from the internal vector.

Without this, the client's reference to the store object would become stale until get() was explicitly called again, leading to potential inconsistencies. Thank you for pointing this out and clarifying the behavior of mutable objects being returned by reference. I will retract my previous suggestion.

[riccardobl]

@gemini-code-assist be more careful in the future, this is a pattern used often in this project.

[gemini-code-assist]

You are absolutely right, @riccardobl. I will strive to be more careful in my analysis, especially concerning common patterns within the project. Thank you for the feedback.

[gemini-code-assist]

The fields numTimesToRepeat and currentCycle are used for counting repetitions. It's better practice to use an integer type (like int) for counters to avoid potential floating-point precision issues and to make the intent clearer.

Suggested change

	private float numTimesToRepeat = -1;
	private float currentCycle = 0;
	private int numTimesToRepeat = -1;
	private int currentCycle = 0;

[gemini-code-assist]

The lightRadiusVector field is initialized to a new Vector2f and is never reassigned to null. Therefore, the null check in simpleUpdate is redundant. Also, the comment contains a few typos.

        // float values like a light's radius cannot be altered by reference like a vector object,
        // so the float value must be manually extracted from the x component of a vector2f and set as radius every frame
        light.setRadius(lightRadiusVector.getX());

[gemini-code-assist]

These imports from java.awt are unused and should be removed.

[gemini-code-assist]

The reset() method does not reset the timeAccrued field. This means if the effect is reused (e.g., in a looping SequencedVectorEffect), the noise will not start from the beginning but continue from where it left off. This might be unexpected behavior for a method named reset. To ensure the effect is fully reset, timeAccrued should be set to 0.

    @Override
    public void reset() {
        super.reset(); 
        originalVectorValues = null;
        timeAccrued = 0;
    }

[gemini-code-assist]

Making this class final prevents it from being extended, which might limit its reusability. If there's no specific security or design reason to make it final, consider removing the final keyword to allow for future extensions.