Skip to content

Fix Cinematic initialization and serialization bugs #2560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix Cinematic initialization and serialization bugs #2560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0ac32d
AnimEvent: fix serialization bug
capdevon Sep 22, 2025
dc88115
Cinematic: fix initialization and serialization events
capdevon Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 77 additions & 39 deletions jme3-core/src/main/java/com/jme3/cinematic/Cinematic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009-2021 jMonkeyEngine
* Copyright (c) 2009-2025 jMonkeyEngine
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -54,46 +54,50 @@
import java.util.logging.Logger;

/**
* An appstate for composing and playing cutscenes in a game. The cinematic
* schedules CinematicEvents over a timeline. Once the Cinematic created it has
* to be attached to the stateManager.
* An AppState for composing and playing cutscenes in a game.
*
* You can add various CinematicEvents to a Cinematic, see package
* com.jme3.cinematic.events
* <p>A cinematic schedules and plays {@link CinematicEvent}s over a timeline.
* Once a Cinematic is created, you must attach it to the `AppStateManager` to
* run it. You can add various `CinematicEvent`s, see the `com.jme3.cinematic.events`
* package for built-in event types.
*
* Two main methods can be used to add an event :
* <p>Events can be added in two main ways:
* <ul>
* <li>{@link Cinematic#addCinematicEvent(float, CinematicEvent)} adds an event
* at a specific time from the cinematic's start.</li>
* <li>{@link Cinematic#enqueueCinematicEvent(CinematicEvent)} adds events
* one after another, with each starting at the end of the previous one.</li>
* </ul>
*
* @see Cinematic#addCinematicEvent(float,
* com.jme3.cinematic.events.CinematicEvent) , that adds an event at the given
* time form the cinematic start.
* <p>Playback can be controlled with methods like:
* <ul>
* <li>{@link Cinematic#play()}</li>
* <li>{@link Cinematic#pause()}</li>
* <li>{@link Cinematic#stop()}</li>
* </ul>
*
* @see
* Cinematic#enqueueCinematicEvent(com.jme3.cinematic.events.CinematicEvent)
* that enqueue events one after the other according to their initialDuration
* <p>Since `Cinematic` itself extends `CinematicEvent`, you can nest cinematics
* within each other. Nested cinematics should not be attached to the `AppStateManager`.
*
* A Cinematic has convenient methods to manage playback:
* @see Cinematic#play()
* @see Cinematic#pause()
* @see Cinematic#stop()
*
* A Cinematic is itself a CinematicEvent, meaning you can embed several
* cinematics. Embedded cinematics must not be added to the stateManager though.
*
* Cinematic can handle several points of view by creating camera nodes
* and activating them on schedule.
* @see Cinematic#bindCamera(java.lang.String, com.jme3.renderer.Camera)
* @see Cinematic#activateCamera(float, java.lang.String)
* @see Cinematic#setActiveCamera(java.lang.String)
* <p>This class also handles multiple camera points of view by creating and
* activating camera nodes on a schedule.
* <ul>
* <li>{@link Cinematic#bindCamera(java.lang.String, com.jme3.renderer.Camera)}</li>
* <li>{@link Cinematic#activateCamera(float, java.lang.String)}</li>
* <li>{@link Cinematic#setActiveCamera(java.lang.String)}</li>
* </ul>
*
* @author Nehon
*/
public class Cinematic extends AbstractCinematicEvent implements AppState {

private static final Logger logger = Logger.getLogger(Cinematic.class.getName());

private Application app;
private Node scene;
protected TimeLine timeLine = new TimeLine();
private int lastFetchedKeyFrame = -1;
private final List<CinematicEvent> cinematicEvents = new ArrayList<>();
private List<CinematicEvent> cinematicEvents = new ArrayList<>();
private Map<String, CameraNode> cameras = new HashMap<>();
private CameraNode currentCam;
private boolean initialized = false;
Expand All @@ -109,14 +113,30 @@ protected Cinematic() {
super();
}

/**
* Creates a cinematic with a specific duration.
*
* @param initialDuration The total duration of the cinematic in seconds.
*/
public Cinematic(float initialDuration) {
super(initialDuration);
}

/**
* Creates a cinematic that loops based on the provided loop mode.
*
* @param loopMode The loop mode. See {@link LoopMode}.
*/
public Cinematic(LoopMode loopMode) {
super(loopMode);
}

/**
* Creates a cinematic with a specific duration and loop mode.
*
* @param initialDuration The total duration of the cinematic in seconds.
* @param loopMode The loop mode. See {@link LoopMode}.
*/
public Cinematic(float initialDuration, LoopMode loopMode) {
super(initialDuration, loopMode);
}
Expand Down Expand Up @@ -221,10 +241,9 @@ public void onPause() {
public void write(JmeExporter ex) throws IOException {
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.

oc.writeStringSavableMap(cameras, "cameras", null);
oc.write(timeLine, "timeLine", null);

}

/**
Expand All @@ -238,12 +257,7 @@ public void write(JmeExporter ex) throws IOException {
public void read(JmeImporter im) throws IOException {
super.read(im);
InputCapsule ic = im.getCapsule(this);

Savable[] events = ic.readSavableArray("cinematicEvents", null);
for (Savable c : events) {
// addCinematicEvent(((CinematicEvent) c).getTime(), (CinematicEvent) c)
cinematicEvents.add((CinematicEvent) c);
}
cinematicEvents = ic.readSavableArrayList("cinematicEvents", null);
cameras = (Map<String, CameraNode>) ic.readStringSavableMap("cameras", null);
timeLine = (TimeLine) ic.readSavable("timeLine", null);
}
Expand Down Expand Up @@ -273,6 +287,7 @@ public void setSpeed(float speed) {
*/
@Override
public void initialize(AppStateManager stateManager, Application app) {
this.app = app;
initEvent(app, this);
for (CinematicEvent cinematicEvent : cinematicEvents) {
cinematicEvent.initEvent(app, this);
Expand Down Expand Up @@ -443,7 +458,7 @@ public KeyFrame addCinematicEvent(float timeStamp, CinematicEvent cinematicEvent
keyFrame.cinematicEvents.add(cinematicEvent);
cinematicEvents.add(cinematicEvent);
if (isInitialized()) {
cinematicEvent.initEvent(null, this);
cinematicEvent.initEvent(app, this);
}
return keyFrame;
}
Expand Down Expand Up @@ -488,7 +503,6 @@ public boolean removeCinematicEvent(CinematicEvent cinematicEvent) {
* @return true if the element has been removed
*/
public boolean removeCinematicEvent(float timeStamp, CinematicEvent cinematicEvent) {
cinematicEvent.dispose();
KeyFrame keyFrame = timeLine.getKeyFrameAtTime(timeStamp);
return removeCinematicEvent(keyFrame, cinematicEvent);
}
Expand Down Expand Up @@ -536,6 +550,9 @@ public void postRender() {
*/
@Override
public void cleanup() {
initialized = false;
clear();
clearCameras();
}

/**
Expand Down Expand Up @@ -591,9 +608,9 @@ public CameraNode getCamera(String cameraName) {
}

/**
* enable/disable the camera control of the cameraNode of the current cam
* Enables or disables the camera control of the cameraNode of the current cam.
*
* @param enabled
* @param enabled `true` to enable, `false` to disable.
*/
private void setEnableCurrentCam(boolean enabled) {
if (currentCam != null) {
Expand Down Expand Up @@ -713,6 +730,15 @@ public Node getScene() {
return scene;
}

/**
* Gets the application instance associated with this cinematic.
*
* @return The application.
*/
public Application getApplication() {
return app;
}

/**
* Remove all events from the Cinematic.
*/
Expand All @@ -725,6 +751,18 @@ public void clear() {
}
}

/**
* Clears all camera nodes bound to the cinematic from the scene node.
* This method removes all previously bound CameraNodes and clears the
* internal camera map, effectively detaching all cameras from the scene.
*/
public void clearCameras() {
for (CameraNode cameraNode : cameras.values()) {
scene.detachChild(cameraNode);
}
cameras.clear();
}

/**
* used internally to clean up the cinematic. Called when the clear() method
* is called
Expand Down
52 changes: 46 additions & 6 deletions jme3-core/src/main/java/com/jme3/cinematic/events/AnimEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009-2021 jMonkeyEngine
* Copyright (c) 2009-2025 jMonkeyEngine
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -41,6 +41,9 @@
import com.jme3.export.JmeExporter;
import com.jme3.export.JmeImporter;
import com.jme3.export.OutputCapsule;
import com.jme3.scene.Node;
import com.jme3.scene.Spatial;

import java.io.IOException;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -56,6 +59,7 @@ public class AnimEvent extends AbstractCinematicEvent {
public static final Logger logger
= Logger.getLogger(AnimEvent.class.getName());

private Spatial model;
/*
* Control that will play the animation
*/
Expand All @@ -73,6 +77,17 @@ public class AnimEvent extends AbstractCinematicEvent {
*/
private String layerName;

/**
* Instantiate a non-looping event to play the named action on the default
* layer of the specified AnimComposer.
*
* @param composer the Control that will play the animation (not null)
* @param actionName the name of the animation action to be played
*/
public AnimEvent(AnimComposer composer, String actionName) {
this(composer, actionName, AnimComposer.DEFAULT_LAYER);
}

/**
* Instantiate a non-looping event to play the named action on the named
* layer of the specified AnimComposer.
Expand All @@ -84,6 +99,7 @@ public class AnimEvent extends AbstractCinematicEvent {
*/
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.

this.composer = composer;
this.actionName = actionName;
this.layerName = layerName;
Expand Down Expand Up @@ -111,6 +127,26 @@ protected AnimEvent() {
public void initEvent(Application app, Cinematic cinematic) {
super.initEvent(app, cinematic);
this.cinematic = cinematic;

if (composer == null) {
if (model != null) {
if (cinematic.getScene() != null) {
Spatial sceneModel = cinematic.getScene().getChild(model.getName());
if (sceneModel != null) {
Node parent = sceneModel.getParent();
parent.detachChild(sceneModel);
sceneModel = model;
parent.attachChild(sceneModel);
} else {
cinematic.getScene().attachChild(model);
}
}
composer = model.getControl(AnimComposer.class);

} else {
throw new UnsupportedOperationException("model should not be null");
}
}
}

/**
Expand Down Expand Up @@ -180,6 +216,13 @@ public void onUpdate(float tpf) {
// do nothing
}

@Override
public void dispose() {
super.dispose();
cinematic = null;
composer = null;
}

/**
* De-serialize this event from the specified importer, for example when
* loading from a J3O file.
Expand All @@ -192,9 +235,8 @@ public void read(JmeImporter importer) throws IOException {
super.read(importer);
InputCapsule capsule = importer.getCapsule(this);

model = (Spatial) capsule.readSavable("model", null);
actionName = capsule.readString("actionName", "");
cinematic = (Cinematic) capsule.readSavable("cinematic", null);
composer = (AnimComposer) capsule.readSavable("composer", null);
layerName = capsule.readString("layerName", AnimComposer.DEFAULT_LAYER);
}

Expand Down Expand Up @@ -269,10 +311,8 @@ public void setTime(float time) {
public void write(JmeExporter exporter) throws IOException {
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.

capsule.write(actionName, "actionName", "");
capsule.write(cinematic, "cinematic", null);
capsule.write(composer, "composer", null);
capsule.write(layerName, "layerName", AnimComposer.DEFAULT_LAYER);
}
}
Loading