diff --git a/crates/skate-mods/src/extensions.rs b/crates/skate-mods/src/extensions.rs index a8085dc6..53aed266 100644 --- a/crates/skate-mods/src/extensions.rs +++ b/crates/skate-mods/src/extensions.rs @@ -4,14 +4,14 @@ use std::collections::BTreeSet; #[derive(Clone, Debug, Deserialize)] #[serde(deny_unknown_fields)] -pub struct MenuOptions { pub title: String, #[serde(default)] pub section: Option, pub items: Vec } +pub struct MenuOptions { pub title: String, #[serde(default)] pub section: Option, #[serde(deserialize_with = "crate::lua_list::list")] pub items: Vec } #[derive(Clone, Debug, Deserialize)] #[serde(deny_unknown_fields)] pub struct MenuItem { pub id: String, pub label: String, #[serde(default)] pub description: String, #[serde(default="enabled")] pub enabled: bool, - #[serde(default)] pub children: Vec, + #[serde(default, deserialize_with = "crate::lua_list::list")] pub children: Vec, } fn enabled() -> bool { true } impl MenuOptions { diff --git a/crates/skate-mods/src/graphics_dynamic.rs b/crates/skate-mods/src/graphics_dynamic.rs index a3e2ba75..79b5e002 100644 --- a/crates/skate-mods/src/graphics_dynamic.rs +++ b/crates/skate-mods/src/graphics_dynamic.rs @@ -70,13 +70,13 @@ pub struct MeshBufferOptions { #[derive(Clone, Debug, Default, Deserialize)] #[serde(deny_unknown_fields)] pub struct MeshBufferWrite { - #[serde(default)] pub positions: Vec<[f32; 3]>, - #[serde(default)] pub normals: Option>, + #[serde(default, deserialize_with = "crate::lua_list::list")] pub positions: Vec<[f32; 3]>, + #[serde(default, deserialize_with = "crate::lua_list::opt_list")] pub normals: Option>, /// Flat RGBA per vertex: `[r,g,b,a, r,g,b,a, ...]`. - #[serde(default)] pub colors: Option>, + #[serde(default, deserialize_with = "crate::lua_list::opt_list")] pub colors: Option>, /// Flat UV pairs: `[u,v, u,v, ...]`. - #[serde(default)] pub uvs: Option>, - #[serde(default)] pub indices: Vec, + #[serde(default, deserialize_with = "crate::lua_list::opt_list")] pub uvs: Option>, + #[serde(default, deserialize_with = "crate::lua_list::list")] pub indices: Vec, } #[derive(Clone, Debug, Deserialize)] diff --git a/crates/skate-mods/src/lib.rs b/crates/skate-mods/src/lib.rs index fda26202..e0ee5bbb 100644 --- a/crates/skate-mods/src/lib.rs +++ b/crates/skate-mods/src/lib.rs @@ -5,6 +5,7 @@ pub mod graphics_dynamic; pub mod presentation; pub mod scene; mod assets; +mod lua_list; pub mod model; mod query; mod schema; diff --git a/crates/skate-mods/src/lua_list.rs b/crates/skate-mods/src/lua_list.rs new file mode 100644 index 00000000..f67dc911 --- /dev/null +++ b/crates/skate-mods/src/lua_list.rs @@ -0,0 +1,163 @@ +//! List fields that a Lua script fills with a table. +//! +//! An empty Lua table has no array part, so mlua hands it to serde as an empty *map*; inside an +//! internally tagged enum such as [`crate::vm::Command`] (serde buffers the whole command first) +//! a plain `Vec` then fails with "invalid type: map, expected a sequence". Every list field a +//! script can fill uses [`list`] or [`opt_list`] instead: a sequence reads as before, and a table +//! whose keys are exactly `1..n` (the empty table included) reads as the list in key order. Any +//! other table is still an error, and fields that are meant to be maps are not touched. +use serde::de::{self, Deserialize, Deserializer, MapAccess, SeqAccess, Visitor}; +use std::{fmt, marker::PhantomData}; + +/// A list field that is always present (missing, `nil` or `{}` = empty). +pub fn list<'de, D: Deserializer<'de>, T: Deserialize<'de>>(d: D) -> Result, D::Error> { + opt_list(d).map(Option::unwrap_or_default) +} + +/// An optional list field: `nil` stays `None`, `{}` is `Some(empty)`. +pub fn opt_list<'de, D: Deserializer<'de>, T: Deserialize<'de>>(d: D) -> Result>, D::Error> { + Option::>::deserialize(d).map(|l| l.map(|l| l.0)) +} + +struct List(Vec); + +impl<'de, T: Deserialize<'de>> Deserialize<'de> for List { + fn deserialize>(d: D) -> Result { + d.deserialize_any(ListVisitor(PhantomData)).map(List) + } +} + +struct ListVisitor(PhantomData); + +impl<'de, T: Deserialize<'de>> Visitor<'de> for ListVisitor { + type Value = Vec; + + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("a list (a table with keys 1..n, or {})") + } + + fn visit_seq>(self, mut seq: A) -> Result, A::Error> { + let mut out = Vec::with_capacity(seq.size_hint().unwrap_or(0).min(4096)); + while let Some(v) = seq.next_element()? { + out.push(v); + } + Ok(out) + } + + fn visit_map>(self, mut map: A) -> Result, A::Error> { + let mut rows: Vec<(u64, T)> = Vec::new(); + while let Some(key) = map.next_key::()? { + let Key::Index(i) = key else { + return Err(de::Error::custom(format_args!("expected a list, found a table with the key {key}"))); + }; + rows.push((i, map.next_value()?)); + } + rows.sort_by_key(|(i, _)| *i); + for (n, (i, _)) in rows.iter().enumerate() { + if *i != n as u64 + 1 { + return Err(de::Error::custom(format_args!( + "expected a list, found a table with the key {i} (list keys run 1..n without gaps)" + ))); + } + } + Ok(rows.into_iter().map(|(_, v)| v).collect()) + } +} + +/// A table key: an integer list index (a Lua integer key; a JSON object's `"1"`), or anything else. +enum Key { + Index(u64), + Other(String), +} + +impl fmt::Display for Key { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Key::Index(i) => write!(f, "{i}"), + Key::Other(s) => write!(f, "{s:?}"), + } + } +} + +impl<'de> Deserialize<'de> for Key { + fn deserialize>(d: D) -> Result { + struct KeyVisitor; + impl Visitor<'_> for KeyVisitor { + type Value = Key; + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("a table key") + } + fn visit_u64(self, v: u64) -> Result { + Ok(Key::Index(v)) + } + fn visit_i64(self, v: i64) -> Result { + Ok(u64::try_from(v).map_or_else(|_| Key::Other(v.to_string()), Key::Index)) + } + fn visit_f64(self, v: f64) -> Result { + Ok(if v.fract() == 0.0 && (0.0..9.0e15).contains(&v) { Key::Index(v as u64) } else { Key::Other(v.to_string()) }) + } + fn visit_str(self, v: &str) -> Result { + Ok(match v.parse::() { + Ok(i) if !v.starts_with('+') && (v == "0" || !v.starts_with('0')) => Key::Index(i), + _ => Key::Other(v.to_owned()), + }) + } + fn visit_bool(self, v: bool) -> Result { + Ok(Key::Other(v.to_string())) + } + fn visit_bytes(self, v: &[u8]) -> Result { + Ok(Key::Other(String::from_utf8_lossy(v).into_owned())) + } + } + d.deserialize_any(KeyVisitor) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use mlua::{Lua, LuaSerdeExt}; + + #[derive(Debug, serde::Deserialize, PartialEq)] + #[serde(tag = "kind", deny_unknown_fields)] + enum Tagged { + T { + #[serde(default, deserialize_with = "list")] + xs: Vec, + #[serde(default, deserialize_with = "opt_list")] + o: Option>, + }, + } + + fn eval(src: &str) -> Result { + let lua = Lua::new(); + let v: mlua::Value = lua.load(src).eval().unwrap(); + lua.from_value(v) + } + + #[test] + fn lua_tables_read_as_lists() { + let t = |xs: Vec, o: Option>| Tagged::T { xs, o: o.map(|o| o.into_iter().map(String::from).collect()) }; + assert_eq!(eval("return {kind='T'}").unwrap(), t(vec![], None)); + assert_eq!(eval("return {kind='T', xs={}, o={}}").unwrap(), t(vec![], Some(vec![]))); + assert_eq!(eval("return {kind='T', xs={3,1,2}, o={'a'}}").unwrap(), t(vec![3, 1, 2], Some(vec!["a"]))); + // Integer keys 1..n in any insertion order are a list. + assert_eq!(eval("local x = {}; x[2] = 5; x[1] = 4; return {kind='T', xs=x}").unwrap(), t(vec![4, 5], None)); + // A gap, a zero key or a named key is not a list. + for bad in ["{[2]=1}", "{[0]=1}", "{a=1}", "{[1.5]=1}", "{[-1]=1}"] { + let e = eval(&format!("return {{kind='T', xs={bad}}}")).unwrap_err().to_string(); + assert!(e.contains("expected a list"), "{bad}: {e}"); + } + assert!(eval("return {kind='T', xs=5}").is_err()); + } + + #[test] + fn json_keeps_reading() { + let j = |v| serde_json::from_value::(v); + assert_eq!(j(serde_json::json!({"kind":"T","xs":[1,2],"o":null})).unwrap(), Tagged::T { xs: vec![1, 2], o: None }); + assert_eq!(j(serde_json::json!({"kind":"T","xs":{},"o":[]})).unwrap(), Tagged::T { xs: vec![], o: Some(vec![]) }); + assert_eq!(j(serde_json::json!({"kind":"T","xs":{"2":7,"1":6}})).unwrap(), Tagged::T { xs: vec![6, 7], o: None }); + assert!(j(serde_json::json!({"kind":"T","xs":{"01":1}})).is_err()); + assert!(j(serde_json::json!({"kind":"T","xs":"x"})).is_err()); + } +} diff --git a/crates/skate-mods/src/presentation.rs b/crates/skate-mods/src/presentation.rs index 437bd153..860564a4 100644 --- a/crates/skate-mods/src/presentation.rs +++ b/crates/skate-mods/src/presentation.rs @@ -120,6 +120,7 @@ pub struct CanvasOptions { pub size: [f32; 2], pub scale: f32, pub visible: bool, + #[serde(deserialize_with = "crate::lua_list::list")] pub items: Vec, } impl Default for CanvasOptions { diff --git a/crates/skate-mods/src/query.rs b/crates/skate-mods/src/query.rs index 79282eae..2702d4d5 100644 --- a/crates/skate-mods/src/query.rs +++ b/crates/skate-mods/src/query.rs @@ -21,7 +21,7 @@ pub struct RaycastOptions { pub max_distance: f32, #[serde(default)] pub filter: RaycastFilter, - #[serde(default)] + #[serde(default, deserialize_with = "crate::lua_list::list")] pub exclude: Vec, } diff --git a/crates/skate-mods/src/scene.rs b/crates/skate-mods/src/scene.rs index f236ff2b..f824feb6 100644 --- a/crates/skate-mods/src/scene.rs +++ b/crates/skate-mods/src/scene.rs @@ -109,6 +109,7 @@ impl GraphicsDefinition { #[serde(default, deny_unknown_fields)] pub struct DetachOptions { /// Candidate FOOT positions in the attached body's local coordinates. + #[serde(deserialize_with = "crate::lua_list::list")] pub candidates:Vec<[f32;3]>, pub ground_snap:f32, pub height:f32, pub radius:f32, } impl Default for DetachOptions { diff --git a/crates/skate-mods/src/vm.rs b/crates/skate-mods/src/vm.rs index 5a0b8d97..52b4a4cf 100644 --- a/crates/skate-mods/src/vm.rs +++ b/crates/skate-mods/src/vm.rs @@ -1,4 +1,4 @@ -use crate::{read_bounded, Manifest}; +use crate::{lua_list::list, read_bounded, Manifest}; use mlua::{HookTriggers, Lua, LuaOptions, LuaSerdeExt, StdLib, Table, VmState}; use serde::Deserialize; use serde_json::Value; @@ -236,7 +236,7 @@ pub enum Command { }, GraphicsMesh { key: String, - #[serde(default)] + #[serde(default, deserialize_with = "list")] deform_nodes: Vec, /// Package-relative GLB path, or empty for a debug box. #[serde(default)] @@ -1380,7 +1380,9 @@ mod graphics_mesh_buffer_tests { ) .eval::() .unwrap(); - assert!(lua.from_value::(value).is_err()); + // `uvs = {}` reads as an empty list (`lua_list`), so the 3-vertex write fails validation + // (two UVs per vertex) instead of deserialisation; `_submit` rejects it either way. + assert!(lua.from_value::(value).is_ok_and(|c| !c.validate())); } #[test] @@ -1711,3 +1713,104 @@ mod deformation_api_tests { assert!(command.validate()); } } + +/// An empty Lua table reaches serde as a map. Every list field a script can fill must still read +/// `{}` as the empty list (`lua_list`), through the real `_submit` path (`Vm` + `api.lua`). +#[cfg(test)] +mod empty_table_lists { + use super::*; + use serde_json::json; + + /// One case per callback (the sandbox has no `pcall`): `ok` cases must submit; `rejected` + /// cases must deserialise and then fail validation, never with a serde "expected a sequence". + #[test] + fn every_list_field_takes_an_empty_table_and_a_list() { + let root = std::env::temp_dir().join(format!("skate-empty-lists-{}", std::process::id())); + std::fs::create_dir_all(&root).unwrap(); + std::fs::write(root.join("main.lua"), r#" + local M = {} + local req = sdk.commands.request + local tri = {{0,0,0},{1,0,0},{0,1,0}} + local cases = { + mesh_named_keys = function() req('m', {kind='graphics_mesh', key='m', deform_nodes={x=1}}) end, + -- graphics (raw requests skip the api.lua wrappers that drop empty tables) + mesh_empty = function() req('m', {kind='graphics_mesh', key='m', deform_nodes={}}) end, + mesh_list = function() req('m', {kind='graphics_mesh', key='m', deform_nodes={'panel'}}) end, + write_list = function() req('w', {kind='graphics_mesh_buffer_write', key='b', data={positions=tri, indices={0,1,2}, + uvs={0,0, 1,0, 0,1}, normals={{0,0,1},{0,0,1},{0,0,1}}, colors={1,1,1,1, 1,1,1,1, 1,1,1,1}}}) end, + write_empty = function() req('w', {kind='graphics_mesh_buffer_write', key='b', data={positions={}, indices={}}}) end, + write_uvs_empty = function() req('w', {kind='graphics_mesh_buffer_write', key='b', data={positions=tri, indices={0,1,2}, uvs={}}}) end, + write_normals_empty = function() req('w', {kind='graphics_mesh_buffer_write', key='b', data={positions=tri, indices={0,1,2}, normals={}}}) end, + write_colors_empty = function() req('w', {kind='graphics_mesh_buffer_write', key='b', data={positions=tri, indices={0,1,2}, colors={}}}) end, + append_empty = function() req('a', {kind='graphics_mesh_buffer_append', key='b', data={positions=tri, indices={}, uvs={}, normals={}, colors={}}}) end, + append_list = function() req('a', {kind='graphics_mesh_buffer_append', key='b', data={positions=tri, indices={0,1,2}}}) end, + wrapper_write = function() sdk.graphics.mesh_buffer_write('b', {positions=tri, indices={0,1,2}, uvs={}}) end, + -- ui + canvas_empty = function() req('c', {kind='ui_canvas', key='c', options={items={}}}) end, + canvas_list = function() req('c', {kind='ui_canvas', key='c', options={items={{key='t', text='hi'}}}}) end, + canvas_wrapper_empty = function() sdk.ui.canvas('c', {items={}}) end, + menu_items_empty = function() sdk.ui.menu('u', {title='T', items={}}) end, + menu_children_empty = function() sdk.ui.menu('u', {title='T', items={{id='a', label='A', children={}}}}) end, + menu_children_list = function() sdk.ui.menu('u', {title='T', items={{id='a', label='A', children={{id='b', label='B'}}}}}) end, + -- player + detach_empty = function() sdk.player.detach{candidates={}} end, + detach_list = function() sdk.player.detach{candidates={{0,0,1}}} end, + -- physics query (deserialised directly, not via a command) + raycast_empty = function() sdk.physics.raycast({0,0,0}, {0,-1,0}, {exclude={}}) end, + raycast_list = function() sdk.physics.raycast({0,0,0}, {0,-1,0}, {exclude={'car'}}) end, + } + function M.on_event(p) cases[p.case]() end + return M + "#).unwrap(); + let manifest: Manifest = serde_json::from_value(json!({ + "id":"tests.empty-lists","api":2,"name":"Empty lists","version":"1.0.0", + "author":"test","description":"test","entry":"main.lua","settings":{}})).unwrap(); + let mut vm = Vm::new(&root, &manifest, &BTreeMap::new(), &Value::Null).unwrap(); + let mut run = |case: &str| -> Result, String> { + let cmds = vm.call("on_event", json!({"case": case}), &json!({}))?; + assert!(cmds.len() <= 1, "{case}"); + Ok(cmds.into_iter().next().map(|c| match c { Command::Request { command, .. } => *command, c => c })) + }; + let mut one = |case: &str| -> Command { run(case).unwrap_or_else(|e| panic!("{case}: {e}")).unwrap_or_else(|| panic!("{case}: no command")) }; + assert!(matches!(one("mesh_empty"), Command::GraphicsMesh { deform_nodes, .. } if deform_nodes.is_empty())); + assert!(matches!(one("mesh_list"), Command::GraphicsMesh { deform_nodes, .. } if deform_nodes == ["panel"])); + assert!(matches!(one("write_list"), Command::GraphicsMeshBufferWrite { data, .. } + if data.positions.len() == 3 && data.indices == [0, 1, 2] && data.uvs.as_ref().is_some_and(|u| u.len() == 6) + && data.normals.as_ref().is_some_and(|n| n.len() == 3) && data.colors.as_ref().is_some_and(|c| c.len() == 12))); + assert!(matches!(one("append_empty"), Command::GraphicsMeshBufferAppend { data, .. } + if data.positions.len() == 3 && data.indices.is_empty() && data.uvs.as_deref() == Some(&[][..]) + && data.normals.as_deref() == Some(&[][..]) && data.colors.as_deref() == Some(&[][..]))); + assert!(matches!(one("append_list"), Command::GraphicsMeshBufferAppend { data, .. } if data.indices == [0, 1, 2] && data.uvs.is_none())); + // api.lua's payload builder still drops an empty uvs table (no UVs). + assert!(matches!(one("wrapper_write"), Command::GraphicsMeshBufferWrite { data, .. } if data.uvs.is_none())); + assert!(matches!(one("canvas_empty"), Command::UiCanvas { options, .. } if options.items.is_empty())); + assert!(matches!(one("canvas_list"), Command::UiCanvas { options, .. } if options.items.len() == 1 && options.items[0].key == "t")); + assert!(matches!(one("canvas_wrapper_empty"), Command::UiCanvas { options, .. } if options.items.is_empty())); + assert!(matches!(one("menu_children_empty"), Command::UiMenu { options, .. } if options.items[0].children.is_empty())); + assert!(matches!(one("menu_children_list"), Command::UiMenu { options, .. } if options.items[0].children[0].id == "b")); + assert!(matches!(one("detach_empty"), Command::PlayerDetach { options } if options.candidates.is_empty())); + assert!(matches!(one("detach_list"), Command::PlayerDetach { options } if options.candidates == [[0., 0., 1.]])); + for case in ["raycast_empty", "raycast_list"] { + assert!(run(case).unwrap_or_else(|e| panic!("{case}: {e}")).is_none(), "{case}"); + } + // Empty where the command needs entries: read fine, refused by validation. + for case in ["write_empty", "write_uvs_empty", "write_normals_empty", "write_colors_empty", "menu_items_empty"] { + let e = run(case).expect_err(case); + assert!(e.contains("Invalid command arguments") && !e.contains("expected a sequence"), "{case}: {e}"); + } + // A table that is not a list is still refused. + let e = run("mesh_named_keys").expect_err("named keys"); + assert!(e.contains("expected a list"), "{e}"); + let _ = std::fs::remove_dir_all(&root); + } + + /// Fields that are maps or free values keep reading `{}` as a map / an object. + #[test] + fn map_fields_keep_empty_tables_as_maps() { + let lua = Lua::new(); + let eval = |src: &str| -> Command { lua.from_value(lua.load(src).eval::().unwrap()).unwrap() }; + assert!(matches!(eval("return {kind='network_state', key='k', value={}}"), Command::NetworkState { value, .. } if value == json!({}))); + assert!(matches!(eval("return {kind='network_state', key='k', value={1,2}}"), Command::NetworkState { value, .. } if value == json!([1, 2]))); + assert!(matches!(eval("return {kind='ui_canvas', key='c', options={}}"), Command::UiCanvas { options, .. } if options.visible && options.items.is_empty())); + } +} diff --git a/docs/hails-additions/20-lua-empty-tables.md b/docs/hails-additions/20-lua-empty-tables.md new file mode 100644 index 00000000..a16d4454 --- /dev/null +++ b/docs/hails-additions/20-lua-empty-tables.md @@ -0,0 +1,41 @@ +# 20. Empty Lua tables in mod command list fields + +## Problem + +A mod that passes an empty table to a list field is refused, for example `sdk.ui.menu('u', {title='T', items={{id='a', label='A', children={}}}})`, a raycast with `{exclude={}}`, `sdk.player.detach{candidates={}}`, or a raw `graphics_mesh` request with `deform_nodes={}`. The command fails with `invalid type: map, expected a sequence`, and the mod loses the whole call. + +## Root cause + +An empty Lua table has no array part, so mlua hands it to serde as an empty **map**. Most commands are an internally tagged enum (`vm::Command`), so serde buffers the whole command first, and a plain `Vec` field then rejects the map. Non-empty list tables have an array part and read fine, which is why this only shows up with `{}`. + +## Evidence + +Found in a muted in-game log check of a mod that sent empty lists: the commands were refused with the serde error above. The unit test below reproduces it for every list field before the change. + +## Change + +- New `crates/skate-mods/src/lua_list.rs`: `list` and `opt_list` serde helpers. A sequence reads as before. A table whose keys are exactly `1..n` (the empty table included) reads as the list in key order. Any other table (a gap, key 0, a named key, a fractional key) is still an error ("expected a list …"). JSON input keeps working, and fields that are meant to be maps aren't touched. +- Every list field a script can fill uses them: + - `vm.rs`: `GraphicsMesh.deform_nodes`; + - `graphics_dynamic.rs`: mesh buffer `positions`, `indices`, `normals`, `colors`, `uvs`; + - `presentation.rs`: canvas `items`; + - `extensions.rs`: menu `items`, `children`; + - `scene.rs`: detach `candidates`; + - `query.rs`: raycast `exclude`. +- `nil` keeps its old meaning: a missing list is empty and a missing optional list is `None`. `{}` in an optional list is `Some(empty)`, so validation still refuses it where entries are required (for example `uvs={}` on a 3-vertex write). + +## Files + +`crates/skate-mods/src/lua_list.rs` (new), `lib.rs`, `vm.rs`, `graphics_dynamic.rs`, `presentation.rs`, `extensions.rs`, `scene.rs`, `query.rs`. + +## Verification + +- `lua_list` unit tests: Lua tables (empty, ordered, out-of-order integer keys, gaps / zero / named / fractional / negative keys refused) and JSON (arrays, `{}`, `{"1":…}`, leading-zero keys refused). +- `vm::empty_table_lists::every_list_field_takes_an_empty_table_and_a_list`: every list field through the real `_submit` path (`Vm` + `api.lua`), with `{}` and a list. Fields that need entries read `{}` and are then refused by validation, never by serde. A table with named keys is still refused. +- `vm::empty_table_lists::map_fields_keep_empty_tables_as_maps`: map / free-value fields still read `{}` as an object. +- `graphics_mesh_buffer_tests`: the `uvs = {}` case now fails validation instead of deserialisation (it is refused either way). +- `cargo test --locked -p skate-mods`: all pass except `skyline_every_component_is_real_and_drives_through_ground_contact`, which needs the Skyline GLB (not in a checkout) and fails on `main` too. `cargo build --locked` is clean. + +## Open questions + +None. The audio moddability PR (#36) carries the same helper for its audio commands. Whichever merges second keeps one copy of `lua_list.rs` (identical file).