Skip to content

Rulesets for table-based function params#86

Open
Rider-Linden wants to merge 7 commits into
mainfrom
rider/fluent
Open

Rulesets for table-based function params#86
Rider-Linden wants to merge 7 commits into
mainfrom
rider/fluent

Conversation

@Rider-Linden

@Rider-Linden Rider-Linden commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Generates a lua interface for functions requiring structured lists of parameters with an initial implementation for ll.ParticleSystem as a POC.

Setting to draft for the moment for comments.
Requires 143

Related PRs:

secondlife/lsl-definitions#143
https://github.com/secondlife/server/pull/2574

local def = {
    emissive = true,
    target_linear = true,
    pattern = PSYS_SRC_PATTERN_EXPLODE,
    start_color = vector(0, 0, 1),
    src_max_age = 10,
    burst_rate = 0.1,
    burst_count = 10,
    target_key = ll.GetOwner()
}

llparticle.ParticleParams.new(def):apply()


local p = llparticle.ParticleParams.new()
-- p.flags = PSYS_PART_INTERP_COLOR_MASK
p.interp_color = true
p.emissive = true
p.pattern = PSYS_SRC_PATTERN_EXPLODE
p.start_color = vector(1, 0, 0)
p.end_color = vector(0, 1, 0)
p.start_alpha = 1.0
p.end_alpha = 1.0
p.src_max_age = 5.0
p.burst_rate = 0.1
p.burst_count = 10
p.burst_radius = 2
p:apply(2)        -- calls ll.ParticleSystem(rules)


print( `emissive = {p.emissive}` )
print(lljson.encode(p))

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a reusable “fluent builder” serialization layer to turn Lua parameter tables into deterministic tag/value rule lists, with initial coverage for particle-system style parameters and collection-valued params.

Changes:

  • Introduces llfluent_builder (C++ implementation + public header) for descriptor-driven table→rules-list serialization, including flag-bit merging and collection semantics.
  • Adds conformance tests exercising particle rule serialization and collection serialization.
  • Updates build packaging/tooling inputs (CMake sources, staged headers, definitions bundle version, and a few build scripts/ignores).

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
VM/src/llfluent_builder.cpp New fluent-builder implementation: descriptor sorting, flag merging, and table serialization into rules lists.
VM/include/llfluent_builder.h Public API for building fluent-builder descriptor defs and registering fluent functions.
tests/SLConformance.test.cpp Adds harness wiring + new test cases that register mock fluent functions and validate serialized output.
tests/conformance/llprim_particle.lua New Lua conformance test validating particle param/flag serialization behavior.
tests/conformance/fluent_builder_collection.lua New Lua conformance test validating collection semantics serialization.
Sources.cmake Wires new fluent-builder sources into the VM build.
init_debian_buster.sh Updates toolchain alternatives and git safe.directory setup in the Debian buster init script.
builtins.txt Updates bundled LSL definitions output (parameter naming and added constants).
build-cmd.sh Stages llfluent_builder.h into the build artifacts include set.
autobuild.xml Bumps lsl_definitions dependency version and associated archive hash/url.
.gitignore Ignores *.tar.bz2 artifacts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread VM/src/llfluent_builder.cpp
Comment thread VM/src/llfluent_builder.cpp Outdated
Comment thread VM/include/llfluent_builder.h
Comment thread tests/SLConformance.test.cpp
Comment thread tests/SLConformance.test.cpp
Comment thread tests/conformance/llprim_particle.lua
Comment on lines +48 to +53
-- Test 5: single flag boolean is merged into the flags integer field.
link, rules = dispatch({ color_interp = true })
assert(#rules == 2, "color_interp: expected 2 elements (flags field only)")
assert(rules[1] == PSYS_PART_FLAGS, "color_interp: tag should be PSYS_PART_FLAGS")
assert(rules[2] == PSYS_PART_INTERP_COLOR_MASK, "color_interp: value should be PSYS_PART_INTERP_COLOR_MASK")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

copilot is not wrong here

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@Rider-Linden Rider-Linden marked this pull request as ready for review July 1, 2026 00:39

@monty-linden monty-linden left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small asks here:

  • C++ include conventions
  • Comments to warn about hazards

Additional comments for consideration.

Comment thread autobuild.xml
@@ -119,17 +119,11 @@
<key>copyright</key>
<string>Copyright (c) 2026, Linden Lab</string>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just noticed this odd LL copyright declaration. Here and in the license file as well. Actual legal entity reference is more in this style:

Copyright (c) 2022, Linden Research, Inc.

assert(#r == 0, "csv nil: output should be empty")

-- Array index order is preserved in the CSV.
r = apply({ whitelist = {"first", "second", "third"} })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When unit testing something like this, maybe not have entries in lexical order.

Comment on lines +48 to +53
-- Test 5: single flag boolean is merged into the flags integer field.
link, rules = dispatch({ color_interp = true })
assert(#rules == 2, "color_interp: expected 2 elements (flags field only)")
assert(rules[1] == PSYS_PART_FLAGS, "color_interp: tag should be PSYS_PART_FLAGS")
assert(rules[2] == PSYS_PART_INTERP_COLOR_MASK, "color_interp: value should be PSYS_PART_INTERP_COLOR_MASK")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

copilot is not wrong here

@@ -0,0 +1,61 @@
#pragma once
#include <stddef.h>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

<cstddef> for c++

Comment on lines +9 to +12
#include <string>
#include <vector>
#include <unordered_map>
#include <algorithm>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

System before application includes.

int field_tag; // tag of the integer field holding the bits, e.g. 0 for "flags"
};

// Opaque handle — definition lives in llfluent_builder.cpp.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not really an opaque. That would be 'void *' with casting. This is forward declaration/incomplete definition.


struct FluentParamDescriptor
{
const char* name; // effective property name (pretty alias or strict fallback)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not owned storage, correct? Owner of FPD needs to guarantee safety. Normally, would consider this and the other occurrences a hazard. This might be short-lived temporary state which allows you to handwave it off, but still. Wondering if 'constexpr'ing these could actually be used to give safety.

def->names[i] = descs[i].name;
def->descs[i] = descs[i];
def->descs[i].name = def->names[i].c_str();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here and below with the moves... we have tons of live pointers into other objects that must not be reallocated in any way or those pointers become invalid. Lay down some clear comments for the next developer if we're going to do this kind of hazard.

Comment on lines +197 to +198
for (int i = 0; i < (int)count; ++i)
def->name_to_index[def->descs[i].name] = i;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Now that we have a sorted list, we could probably binary search it faster than the amortized cost of a small unordered map. (Not a serious request but vector beats everything at small numbers.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants