From a2ada5a5feeef838271728851975479582efeb53 Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 12 Nov 2026 13:14:16 +0530 Subject: [PATCH] #3087: count every parameter a placeholder walks, and popup names by length A definition of parameters that are not kept, shared by many instances, was walked again for each one without spending the budget; long popup item names were kept per instance with each item counted once. --- crates/aep/src/import/placeholder.rs | 28 +++++++++++++++------- crates/aep/tests/it/import/placeholders.rs | 23 ++++++++++++++++++ docs/aep-import.md | 3 ++- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/crates/aep/src/import/placeholder.rs b/crates/aep/src/import/placeholder.rs index a12ffa6c5..4186c9e28 100644 --- a/crates/aep/src/import/placeholder.rs +++ b/crates/aep/src/import/placeholder.rs @@ -11,8 +11,9 @@ //! does). Layer parameters name their layer (their keys and expression are reported). Paths and //! arbitrary data are not kept (reported when they hold something). //! -//! The parameters a placeholder holds are bounded per import ([`MAX_COST`]): definitions can be -//! shared by many instances, so a small file could otherwise ask for very many properties. Once +//! The parameters placeholders walk, kept or not, and their popup items (by name length) are +//! bounded per import ([`MAX_COST`]): definitions can be shared by many instances, so a small file +//! could otherwise ask for very many properties. Once //! the budget is spent, later effects without a counterpart are reported without a placeholder. use effectcraft_keyframe::Value; @@ -24,12 +25,16 @@ use crate::import::props::{Kind, import_expression, import_keys}; use crate::import::report::{Collector, LossCategory}; use crate::{Effect as AepEffect, EffectParam, ParamKind, ParamValue}; -/// Most placeholder properties and popup items one import creates. +/// Most placeholder parameters (kept or not) and popup items one import walks. pub(crate) const MAX_COST: usize = 200_000; /// Most items read from one placeholder popup. const MAX_POPUP_ITEMS: usize = 1024; +/// A popup item costs one more for each of these bytes in its name, so the names kept stay +/// bounded too. +const NAME_BYTES: usize = 16; + /// The placeholder of `fx`, with `params` (its parameter definitions) as properties. pub(crate) fn placeholder( fx: &AepEffect<'_, '_>, @@ -45,26 +50,33 @@ pub(crate) fn placeholder( let mut group = ids.group(&segment(fx.match_name()), shown); group.kind = GroupKind::Effect { effect: fx.match_name().to_string() }; let here = format!("{}: '{}' effect '{shown}' ({})", cx.comp, cx.layer, fx.match_name()); - for p in params.iter().filter(|p| !carries_nothing(p)) { + for p in params { + // Every parameter walked counts, kept or not: a definition shared by many instances is + // otherwise walked again for each one. + if carries_nothing(p) { + col.placeholder_cost = col.placeholder_cost.saturating_add(1).min(MAX_COST); + continue; + } if col.placeholder_cost >= MAX_COST { col.lose(LossCategory::Effects, format!("{here}: parameters past the import's {MAX_COST} placeholder parameters not kept")); break; } let label = param_label(p); let Some((kind, ui, default)) = shape(p, cx.size) else { + col.placeholder_cost = col.placeholder_cost.saturating_add(1); if p.stored().is_some() { col.lose(LossCategory::Effects, format!("{here}: parameter {label} not kept (type {:?})", p.kind)); } continue; }; - let items = match &ui { - ParamUi::Popup { options } => options.len(), - _ => 0, + let (items, names) = match &ui { + ParamUi::Popup { options } => (options.len(), options.iter().map(|o| 1 + o.len() / NAME_BYTES).fold(0, usize::saturating_add)), + _ => (0, 0), }; if p.kind == ParamKind::Popup && popup_len(p).is_some_and(|n| n > items) { col.lose(LossCategory::Effects, format!("{here}: parameter {label}: the items past the first {MAX_POPUP_ITEMS} not kept")); } - let cost = col.placeholder_cost.saturating_add(items).saturating_add(1); + let cost = col.placeholder_cost.saturating_add(names).saturating_add(1); if cost > MAX_COST { // The budget is spent: nothing more is built, here or in later effects (definitions // shared by many instances would otherwise still be walked item by item). diff --git a/crates/aep/tests/it/import/placeholders.rs b/crates/aep/tests/it/import/placeholders.rs index 679d508af..01f1cbc43 100644 --- a/crates/aep/tests/it/import/placeholders.rs +++ b/crates/aep/tests/it/import/placeholders.rs @@ -190,6 +190,29 @@ fn an_effect_past_the_spent_budget_gets_no_placeholder() { assert!(found.iter().any(|d| d.contains("kept without rendering") && !d.contains("more like it")), "the kept instance alone: {found:?}"); } +/// Parameters a placeholder does not keep (markers, arbitrary data) still spend the budget: a +/// definition of very many of them, shared by many instances, is not walked again for each one. +#[test] +fn parameters_not_kept_still_spend_the_budget() { + let defs = (0..200_000).map(|i| other(if i % 2 == 0 { K::NoData } else { K::ArbitraryData }, "X", Default::None)).collect(); + let fx = Fx { match_name: "Odd Fx", name: "Odd", defs, values: vec![], data: vec![], keys: vec![] }; + let (_, r) = import_bytes(&twice(&bytes_with(&fx), "Odd Fx"), "Odd Fx"); + let found = losses(&r, LossCategory::Effects, "Odd Fx"); + assert!(found.iter().any(|d| d.contains("not kept (past the import's placeholder parameters)")), "{found:?}"); +} + +/// Long popup item names spend the budget by their length, so the names kept stay bounded. +#[test] +fn long_popup_item_names_spend_the_budget() { + let long = "x".repeat(1600); + let items: &'static [&'static str] = + Box::leak((0..1024).map(|i| &*Box::leak(format!("{long}{i}").into_boxed_str())).collect::>().into_boxed_slice()); + let fx = Fx { match_name: "Long Fx", name: "Long", defs: vec![popup("A", 1, items), popup("B", 1, items)], values: vec![], data: vec![], keys: vec![] }; + let (g, r) = placeholder_of(&fx); + assert_eq!(g.props().count(), 1); + assert!(losses(&r, LossCategory::Effects, "Long Fx").iter().any(|d| d.contains("placeholder parameters"))); +} + /// `bytes` with the layer's instance of `match_name` (its `tdmn` and `sspc`) repeated after it. fn twice(bytes: &[u8], match_name: &str) -> Vec { let mut file = parse(bytes).unwrap(); diff --git a/docs/aep-import.md b/docs/aep-import.md index 08efa6123..ffaa3f733 100644 --- a/docs/aep-import.md +++ b/docs/aep-import.md @@ -333,7 +333,8 @@ parameter gets the file's value: pseudo effect's `Pseudo/0.25-0001`). Paths and arbitrary data are not kept; a popup whose definition is borrowed, or whose default slot holds no item, and that has no value record takes its first item (reported). One import keeps at - most 200 000 placeholder parameters and popup items (a popup at most 1024 items); the rest is + most 200 000 placeholder parameters and popup items (a popup at most 1024 items; parameters it + does not keep count, and an item one more per 16 bytes of its name); the rest is reported, and once that is spent a later effect without a counterpart arrives without a placeholder (reported). A placeholder is still reported in `effects` and counted in `imported.placeholderEffects`; .aep export reports it and leaves it out.