Skip to content

Conversation

capdevon
Copy link
Contributor

@capdevon capdevon commented Sep 22, 2025

Refactors and Improves the Cinematic class

This pull request enhances the Cinematic class, focusing on better documentation, serialization improvements, and memory management. Key updates include:

  • Improved Javadoc: Expanded and clarified class-level and method-level documentation for better developer understanding and usability.
  • Serialization Refactor: Replaces custom array-based serialization of cinematic events with more robust SavableArrayList serialization/deserialization.
  • App Reference: Adds an Application field to the class, initialized during initialize() and passed to event initialization for consistency.
  • Camera Management: Introduces a new clearCameras() method to properly detach and clear all camera nodes from the scene, now called from cleanup() to avoid memory leaks.
  • Cleanup Enhancements: Ensures the cinematic is properly reset on cleanup by resetting the initialized flag, clearing events, and removing camera nodes.
  • Constructor and Parameter Documentation: Adds and improves JavaDoc for constructors and methods, clarifies parameter usage, and updates copyright.
  • Minor Code and Comment Cleanups: Updates comments, fixes typos, improves formatting, and makes the cinematicEvents list non-final for deserialization.

These changes improve maintainability, clarity, and the robustness of cinematic lifecycle management in the engine.

Enhance AnimEvent Serialization and Model Handling

This PR introduces improvements to the AnimEvent class in the com.jme3.cinematic.events package by enhancing how animation events are associated with their models and how they are serialized/deserialized. The changes ensure more robust handling of AnimComposer and the associated Spatial model, especially when saving and loading animation events.

Key Changes

  • Model Reference Added:
    Introduced a private Spatial model field to AnimEvent to keep track of the model associated with the animation event.
  • Constructors Updated:
    • Added a new constructor to allow instantiation with default animation layer.
    • Modified constructors to initialize the model field from the AnimComposer.
  • Initialization Logic Enhanced:
    In initEvent, if the composer is not set but the model is, the code attempts to retrieve the correct AnimComposer from the model, ensuring reliable initialization even after deserialization.
  • Serialization Improvements:
    • The model field is now written to and read from the JME capsules, replacing the direct serialization of cinematic and composer fields (which are no longer serialized).
    • This approach improves the resilience of event saving/loading, especially when objects are reconstructed from files.
  • Dispose Method Implemented:
    Added a custom dispose() method to clean up references to cinematic and composer when the event is disposed, aiding memory management.
  • Copyright Update:
    Updated the copyright year.

Motivation

These changes address issues related to the persistence and restoration of animation events across sessions, improving the reliability of cinematic workflows in jMonkeyEngine. By serializing the model reference and reconstructing the composer as needed, the risk of null references or mismatched state is reduced.

Impact

  • Improves the robustness of cinematic event handling.
  • Enhances serialization/deserialization for animation events.

@codex128 codex128 added the Refactoring and Cleanup This PR is all about improving code quality and javadoc. label Sep 22, 2025
@codex128 codex128 self-requested a review September 22, 2025 16:29
*/
public AnimEvent(AnimComposer composer, String actionName,
String layerName) {
this.model = composer.getSpatial();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any instance where composer.getSpatial() returns null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I see you have null checks later.

super.write(exporter);
OutputCapsule capsule = exporter.getCapsule(this);

capsule.write(model, "model", null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saving the model here will create a duplicate model on load.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see that you're switching out the original for the duplicate loaded from here. This is still really dangerous for several reasons:

  • Possible infinite recursion: event saves model, which (directly or indirectly) saves the event.
  • At least double memory footprint for the model.
  • Multiple spatials are allowed to have the same name.
  • Something else may be acting on the original spatial, and expecting it to still be attached.

It'd be ideal if model's reference could be saved instead of the model itself. Imo, that is a serious limitation of the serializer.

super.write(ex);
OutputCapsule oc = ex.getCapsule(this);
oc.write(cinematicEvents.toArray(new CinematicEvent[cinematicEvents.size()]), "cinematicEvents", null);
oc.writeSavableArrayList((ArrayList) cinematicEvents, "cinematicEvents", null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cinematicEvents is expected to be an ArrayList, just make it an ArrayList instead of a List.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring and Cleanup This PR is all about improving code quality and javadoc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants