You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add behavioural tests for org.almostrealism.music.pattern - #593
Adds thirteen test classes (80 methods) covering the pattern package:
element scheduling and duration strategies, the layer hierarchy,
deterministic position/chord selection functions, the element factory
and seed generation, MIDI export and render destinations, the note audio
cache ownership contract, PatternLayerManager layering and settings,
PatternSystemManager bookkeeping, and chord progression regions.
PatternRenderTest renders a pattern end to end over a generated sample
and checks that buffer-by-buffer rendering through PatternAudioBuffer
(note cache and eviction) matches a single full render, for both the
per-note and batched render paths.
Local JaCoCo line coverage for the package rises to 87.3% (1379/1579);
the ledger row is appended to tools/coverage-data/coverage-history.tsv.
Adds thirteen test classes (80 methods) covering the pattern package:
element scheduling and duration strategies, the layer hierarchy,
deterministic position/chord selection functions, the element factory
and seed generation, MIDI export and render destinations, the note audio
cache ownership contract, PatternLayerManager layering and settings,
PatternSystemManager bookkeeping, and chord progression regions.
PatternRenderTest renders a pattern end to end over a generated sample
and checks that buffer-by-buffer rendering through PatternAudioBuffer
(note cache and eviction) matches a single full render, for both the
per-note and batched render paths.
Local JaCoCo line coverage for the package rises to 87.3% (1379/1579);
the ledger row is appended to tools/coverage-data/coverage-history.tsv.
BatchedPatternLayerRenderer.bucketFor returned the largest bucket (512)
for larger note counts, so a sub-window with more than 512 overlapping
notes overran the scalar staging array and the bound source rows.
bucketFor now rejects oversized counts and dispatchBatched splits each
sub-window into maxBucket()-sized chunks that accumulate into the same
destination slice.
NoteDurationStrategy.NO_OVERLAP produced a negative length when no next
position after the note was available; it now falls back to the
original duration.
Tests: PatternRenderTest gains an oversized-batch regression test,
kernel-compiling render tests are depth-gated at 2, and the buffered
batched test checks that the buffered render itself dispatches the
batched renderer. BatchedNoteInputsLayoutTest and PatternElementTest
are updated for the new contracts.
…ation
Address three defects in the music pattern renderer surfaced during review of
the new behavioural coverage:
- NoteAudioCache keyed entries by frame offset alone, so coincident notes
(chords, layered voices) collided in the per-note buffered render path and
every note after the first reused the first note's audio. The cache key is
now a composite (offset, identity); RenderedNoteAudio carries a stable
cacheIdentity set from (element, details) by ScaleTraversalStrategy, and
PatternFeatures.renderNotes passes it through get/put.
- BatchedPatternLayerRenderer counted one batched dispatch per dispatchBatched
call, underreporting sub-window and oversized-chunk kernels. The counter is
now incremented per dispatchWindow, matching its "kernel dispatches" contract.
- PatternNoteFactory.apply indexed choices[i] without checking length, leaking
an ArrayIndexOutOfBoundsException; it now validates one selection per layer
and throws IllegalArgumentException.
Adds/extends tests: coincident chord identities, cache distinctness, the
cacheIdentity accessor, oversized-batch dispatch counts, and factory validation.
A layer's NoteAudioCache serves both stereo channels, and
ElementVoicingDetails equality ignores the channel, so a note that
continued across buffers could reuse the other channel's cached audio.
The per-note cache identity set in ScaleTraversalStrategy now includes
the stereo channel.
The identity is built with Arrays.asList rather than List.of because
the channel can be null: RiseManager renders with a default
NoteAudioContext that has no channel, and List.of rejects null
elements, which would throw for every rendered note.
Adds ScaleTraversalStrategyTest#stereoChannelsHaveDistinctCacheIdentities.
Update the javadoc of ElementVoicingDetailsTest#stereoChannelDoesNotAffectIdentity
to explain that the stereo channel is excluded from ElementVoicingDetails
equality/hashCode on purpose: LEFT/RIGHT renders are kept distinct in
NoteAudioCache by the (element, details, stereoChannel) cache identity built in
ScaleTraversalStrategy.createRenderedNote, not by these details. The assertions
are unchanged; this is a comment-only clarification.
The oversized-batch regression creates a percussive pattern (addPattern(..., false)), so it exercises only dispatchWindowPercussion. This new chunking also routes oversized melodic batches through the separate dispatchWindow packing/kernel path; add an oversized melodic render assertion so both affected branches are covered.
The PR description presents this as a test-only coverage change, but this diff also changes the public cache API/key semantics and adds production fixes for oversized dispatches, duration fallback, and note-factory validation. Update the title/description to disclose these runtime and API changes so reviewers and release notes reflect the actual scope.
Update cache key documentation to include cacheIdentity
The public class documentation above still says rendered audio is cached “by note offset,” which now omits the required cacheIdentity portion of the key. Update that rendering-process description so callers do not implement the old collision-prone contract.
The class-level "Rendering Process" javadoc still described the rendered
audio as "cached by note offset", which omitted the cacheIdentity half of
the composite NoteAudioCache key introduced on this branch. Update it to
state the cache is keyed by (offset, cacheIdentity) and that the identity
is what keeps coincident notes (chords, layered voices, stereo channels)
at the same offset from sharing a cache entry.
Documentation only; no behavioural change.
The new exception embeds the invalid note count and bucket limit in its message. The repository's error-handling rule requires field values to be carried separately by a custom exception; use a stable single-sentence message or introduce a typed exception with count/limit fields, and update the message-based test accordingly.
Avoid embedding validation counts in exception message
This exception message embeds the expected and received counts, contrary to the repository rule that field values in errors be stored separately in a custom exception. Since callers only need the validation contract here, use a stable single-sentence message (or add a typed exception with count fields).
BatchedPatternLayerRenderer.bucketFor and PatternNoteFactory.apply now throw
simple, single-sentence messages without embedded field values, following the
project's error-handling convention. BatchedNoteInputsLayoutTest and
PatternElementFactoryTest assert the exact messages.
- init() allocates the manager-owned volume independently of any active
Heap; pack() goes through PackedCollection.factory() and would return a
heap alias that destroy() cannot release.
- warmNoteCache() destroys each discarded evaluation output when no heap
is active; Heap.stage() is a no-op without one, so the previous code
left the result to the garbage collector.
- clear() advances a pattern generation, and render operations built by
sum() refuse to run once it has moved on, throwing
IllegalStateException instead of evaluating against pattern managers
whose native memory clear()/setSettings()/destroy() released.
Adds regression tests for heap-independent volume allocation, rendering
after a warm-up, and rejection of a stale render operation.
AudioScene.destroy() now releases each pattern's note-audio cache, but a
PDSL real-time runner's render-ahead producer thread keeps rendering
patterns (mutating those caches) until the runner is reset. Destroying a
scene whose runner was still live raced with that thread and threw a
ConcurrentModificationException in NoteAudioCache.clear().
PatternRenderBuffers now tracks the scene's PatternRenderStreams (the
runner registers each one through AudioScene.registerRenderStream), and
AudioScene.destroy() stops them before destroying the pattern system.
The streams' ring storage is also released with the render buffers.
Adds PatternRenderBuffersTest covering the producer shutdown.
The reason will be displayed to describe this comment to others. Learn more.
Partly addressed. PatternLayerManager.destroy() now takes the same per-manager renderLock that the sum() body holds. If a render has already entered a manager's sum(), clear() now waits for it to finish before releasing that manager's caches and automation data. It no longer frees memory under a running render. One window is still open: a tick can pass the generation check and reach a manager's sum() only after clear() has destroyed it. Closing that needs the generation check and the manager calls under one system-level lock (or stopping the producer before replacement), which is a lifecycle decision for the render-ahead model. I've left it open.
The reason will be displayed to describe this comment to others. Learn more.
Fixed. PatternLayerManager now carries a destroyed flag, set under renderLock at the end of destroy() (after the note-audio cache, automation parameter collections and batched renderer are released). The sum() operation body — which already acquires the same renderLock — now checks the flag first and throws IllegalStateException if it is set, before touching any cache or renderer state.
Because destroy() sets the flag and sum() reads it under the one renderLock, the two are mutually exclusive: a render-ahead tick that passes PatternSystemManager.sum()'s generation guard and only then has the owning thread call PatternSystemManager.clear() either (a) completes its render before clear() acquires the lock to tear the manager down, or (b) acquires the lock after teardown, observes destroyed, and refuses to run rather than dereferencing the released automation/cache memory. This closes the window without resolving the broader single-owner render-ahead design question.
I chose to throw (not skip) to match the stale-operation contract of PatternSystemManager.sum(), which already throws IllegalStateException when the generation has advanced; the message likewise says the operation is stale. Covered by the new regression test PatternLayerManagerTest#renderOperationIsRejectedAfterManagerDestroyed: a manager renders while live, then running the same operation after destroy() throws.
The reason will be displayed to describe this comment to others. Learn more.
Fixed. createPdsl now creates the PatternRenderStream and calls scene.registerRenderStream(...) immediately before returning the TemporalCellular, after PDSL parsing, model compilation and the build-time forward pass. The stream is referenced only inside the returned runner, so moving it changes nothing on the success path, and a construction failure above it now never allocates or registers the stream — no orphaned stream or native ring is left against the scene.
The remaining render-ahead lifecycle items you raised on this change (the 2s join in stop() not guaranteeing producer termination before patterns.destroy(), several streams sharing one manager's NoteAudioCache, the clear()/producer race, and the per-build consolidated-root buffer not being tracked with its stream) are genuine but require a single-owner/synchronization design decision for the render-ahead model rather than a local change; they're left for the author.
PatternSystemManager.sum() moves the pattern-generation guard ahead of the
empty-channel early return, and addPattern() now advances the pattern
generation as clear() does. A render operation built for a channel that had
no patterns at build time is therefore rejected once a later addPattern()
changes the pattern set, instead of silently continuing to render as an empty
channel, so a caller rebuilds the operation and picks up the new pattern.
Add PatternRenderTest#renderOperationForEmptyChannelIsRejectedAfterPatternAdded
covering the empty-channel case, and give two ScaleTraversalMidiTest methods a
finally block that destroys the gathered RenderedNoteAudio so their offset
arguments are not leaked.
These chord destinations own native offset arguments and are never destroyed after the identity assertions. Add deterministic cleanup after the assertions.
This issue also appears on line 411 of the same file.
The reason will be displayed to describe this comment to others. Learn more.
Addressed at the manager level. PatternLayerManager now has a per-manager renderLock. It is held by the whole sum() body (epoch/evict/clear, sumInternal, end-of-render clear) and by every path that releases or rebuilds render state: releaseRenderCaches, removeLayer, clear, refresh, setExplicitElements and destroy. Concurrent producers rendering the same manager are serialized, so the NoteAudioCache and gather-cache maps are never mutated concurrently. A cache release or teardown also waits for an in-flight render instead of destroying audio it is using. This race was what CI hit: a ConcurrentModificationException in NoteAudioCache.clear from assignGenome → refresh() while the render-ahead producer was inside sum().
…sets
PatternSystemManager.getPatterns() now returns an unmodifiable live view, so
the pattern set can only change through addPattern(), clear() or
setSettings(), which advance the pattern generation that stale sum()
operations are checked against. PatternSystemManagerTest gains
patternListViewIsReadOnly to cover the view.
Two ScaleTraversalStrategyTest methods now destroy the RenderedNoteAudio
instances they gather, releasing each note's offset argument.
AudioSceneRealtimeRunner.createPdsl built the PatternRenderStream (which
allocates a native ring) and registered it against the scene before the
remaining fallible construction steps ran. A failure in those steps left no
runner for the caller but kept the stream and its ring registered on the scene
until scene teardown.
Move the stream creation and registration to immediately before the returned
TemporalCellular, after every fallible step. The stream is referenced only
inside that returned object, so the happy path is unchanged; a construction
failure now never allocates or registers the stream.
PatternLayerManager now releases its note-audio cache and the batched
renderer's gather cache from refresh(), clear(), removeLayer() and
setExplicitElements(). Those run on the thread that assigns a genome,
while the render-ahead producer thread may be inside sum() reading and
filling the same plain HashMaps. The race threw
ConcurrentModificationException on the caller side, and on the
producer side it killed the render-ahead thread, so renders came out
silent and the consumer parked forever waiting for a buffer.
Add a per-manager render lock held by the sum() body and by every
release / hierarchy-mutation path (including destroy()). A release now
waits for an in-flight render to finish. A render never sees a
half-cleared cache, destroyed note audio, or a partially rebuilt layer
hierarchy. A runner has a single producer thread, so the lock is
uncontended during steady-state rendering.
Close the one use-after-free window the PatternSystemManager.sum()
generation guard left open: a render-ahead tick can pass that guard and
only then have the owning thread destroy the captured PatternLayerManager
through PatternSystemManager.clear(), so the tick would render against the
manager's released automation parameter collections and batched renderer.
PatternLayerManager now carries a `destroyed` flag, set under renderLock
at the end of destroy(). The sum() operation body, which already holds
renderLock, checks the flag first and throws IllegalStateException if the
manager was torn down, matching the stale-operation contract of
PatternSystemManager.sum(). Because the flag is set and read under the one
renderLock, destroy() and a render are mutually exclusive: the render
either completes before teardown or observes the flag and refuses to run.
Add PatternLayerManagerTest#renderOperationIsRejectedAfterManagerDestroyed
covering that a live manager renders and the same operation throws after
destroy().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds thirteen test classes (80 methods) covering the pattern package:
element scheduling and duration strategies, the layer hierarchy,
deterministic position/chord selection functions, the element factory
and seed generation, MIDI export and render destinations, the note audio
cache ownership contract, PatternLayerManager layering and settings,
PatternSystemManager bookkeeping, and chord progression regions.
PatternRenderTest renders a pattern end to end over a generated sample
and checks that buffer-by-buffer rendering through PatternAudioBuffer
(note cache and eviction) matches a single full render, for both the
per-note and batched render paths.
Local JaCoCo line coverage for the package rises to 87.3% (1379/1579);
the ledger row is appended to tools/coverage-data/coverage-history.tsv.