Feature: Lightbox V2 - #361
Conversation
Replace the stalled WIP floater (dead tonemapper/sharpen machinery wired
to removed settings, fixed-pixel 400x380 XUI) with a resizable
Look/Lens/Scene shell where each tab is an accordion of effect sections
loaded from its own panel XML. Rows bind straight to settings via
control_name; the only C++ glue is a generic Vec3/Color3 component
binder driven by the "vec3_<Setting>_<idx>" widget naming contract and
a data-driven per-section reset that walks a section for its bound
controls instead of hardcoding key lists.
The Look tab ships the proof-of-pattern sections: exposure/tonemapper
(with per-operator params and auto-exposure under Advanced), color LUT,
split toning (color_swatch on a Color3 setting), and lift/gamma/gain
(Vec3 spinner rows). Lens and Scene are placeholders for the next
phases.
Declare RenderDynamicExposure{Enabled,SpeedError,SpeedTarget}, which
the pipeline already reads but settings.xml never declared, and make
them and the existing auto-exposure knobs persistent now that they are
user-facing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
An accordion_tab rect is its expand height, and fit_panel squeezes the inner panel into what remains below the 25px header plus 2+2 padding. The sections sized tab == panel, so every section lost its bottom 29px and the tall Advanced tab drew its overflow across the sections below it (panels do not clip children). Tabs are now panel height + 29 and the contract is documented in the panel header comment. Per-operator tonemapper rows (ACES Boosted, Reinhard, Filmic, AgX white points and contrast) previously stayed editable regardless of the selected operator; they and their reset buttons now enable only while their operator is active, driven by a connection on AlchemyRenderTonemapType. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
The advanced tab was 32px short: slider rows chain top_pad from their reset button, which hangs 1px below the slider, so slider+reset rows pitch 26px rather than the 25 the original sum assumed. Content ends at 290, giving panel 291 / tab 320 (in-viewer verified). The height comment now spells out the row-pitch rule. color_swatch reserves a label strip below the color area by default (label_height -1), which left a 24px-tall swatch roughly one pixel of color; label_height="0" gives the color the full rect, matching the environment-adjust floater swatches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
LLColor4(LLSD) reads element 3 of a 3-element Color3 array as 0, so any color_swatch bound to a Color3 control drew fully transparent (checkerboard) even though the RGB round-trip worked. Both the Lightbox tint swatches and the Debug Settings floater feed swatches through setValue, so forcing alpha to 1 for 3-element arrays at that single chokepoint fixes the display everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
The previous fix only forced alpha for 3-element values, but the first
pick made under alpha 0 had already written a 4-element array into the
Color3 control ("keep current alpha" in onColorChanged), so the stored
value carried an explicit 0 that the size==3 guard never caught, and
every later pick preserved it.
Display now also treats any swatch bound to a TYPE_COL3 control as
opaque, and the picker write path stores only three components for
such controls, healing poisoned values on the next commit. Debug
Settings feeds its swatch the parsed 3-element color instead of the
raw stored LLSD so its display is shape-independent too.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Adds the Basic Grade section (brightness/contrast/saturation/vibrance, with highlights/shadows/black point/white point/hue shift under Advanced), White Balance (CCT and Duv), the rest of Split Toning (highlight tint, balance; midtone tint and amount under Advanced), and the per-channel Tone Curve (toe/shoulder/strength as Vec3 spinner rows). Ranges and tooltips come from the setting Comments. populateLUTCombo now scans the bundled app_settings/colorlut directory as well as the user colorlut directory (separator between them, matching the order the renderer resolves names in), replacing the hardcoded bundled item list so newly shipped LUTs appear without XML edits; bundled entries are labeled by filename stem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Two advanced rows did not justify a separate accordion tab; midtone tint and amount now live at the bottom of Split Toning and the Advanced sibling is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Fourteen accordion sections covering the optical effects. Depth of field, HDR bloom, legacy glow, lens flare, chromatic aberration, and vignette each pair an essentials section with an Advanced sibling; film grain and sharpen/dither fold their short tails into a single section. The bloom/glow pair expresses the HDR fork by greying: HDR bloom rows enable on RenderBloomHDR, legacy glow rows on its inverse, halation rows on RenderBloomHalation, and the DoF parameter rows on RenderDepthOfField. Glow luminance/warmth weights and the vignette center ride the existing Vec3 spinner binder; no new C++. Ranges, defaults, and tooltip text come from the setting Comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Bloom base resolution becomes a dropdown (Full / 3/4 / Half / Quarter / Eighth) instead of a bare 0-4 slider. Lens flare advanced groups get bold headers with divider lines so the categories read at a glance. Chromatic aberration merges into a single section since its essentials were one slider. Film grain "Range" is relabeled "Luma range". The dither toggles leave the floater entirely - dithering is a de-banding feature that should only ever be disabled for debugging, so it stays Debug Settings-only - and the section is now just "Sharpen". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
RenderDisablePostProcessing only takes effect while the build tools floater is open (no_post gate in LLPipeline::colorCorrect), so the checkbox promised a global bypass it cannot deliver. The develop menu entry remains for the build-mode use case; the topbar strip is reserved for the Looks bar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
RenderBloomHDR has no renderer-side consumer on this branch, so the bloom/glow sections greyed against a setting that changes nothing. Until that is resolved, gate both sections on RenderHDREnabled - the key the renderFinalize fork actually tests - so the greying reflects which path really runs. The dead RenderBloomHDR toggle row is removed, and RenderHDREnabled is deliberately not offered as a toggle here since flipping it cascades probe resets, buffer reallocation, and a shader rebuild; it stays a read-only gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Ten accordion sections for render quality and performance. Anti-aliasing, reflections, shadows/AO, and performance each pair essentials with an Advanced sibling (grouped with bold headers and divider lines); Preview & Accessibility and Output are single sections. Every enum-valued setting is a dropdown, with items and slider ranges lifted from the advanced graphics preferences so the two UIs agree. SMAA predication rows gate on the predication toggle, SSR march knobs on the SSR toggle, SSAO tuning on the SSAO toggle, and mirror resolution/rate on RenderMirrors. RenderSSAOEffect rides the Vec3 spinner binder. Per earlier decisions there is no RenderHDREnabled toggle and no RenderQualityPerformance macro slider; tooltips note which toggles rebuild shaders or reallocate buffers, and the unlistened RenderHighPrecisionPostProcess / 10-bit SDR flags carry restart notes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Anti-Aliasing loses its Advanced sibling (the SMAA predication family is default-tuned and stays Debug Settings-only). Mirrors gets its own section holding the toggle plus resolution and update rate, moved out of the reflections Advanced tab; probe Coverage moves up into the Reflections essentials. The Output section is removed entirely - the high-precision and 10-bit flags are too touchy to expose. Shadow resolution scale becomes an x2 / x1 / x0.5 dropdown instead of a slider so rapid drags cannot thrash shadow-map reallocation (handleShadowsResized fires per change). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
RenderSSAOEffect is a value multiplier, a saturation multiplier (HSV, for fully occluded areas, blending with the original color under partial occlusion), and an unused third component - so the generic X/Y/Z spinner triplet becomes two labeled rows, "Occluded value" and "Occluded saturation", each a single component spinner through the Vec3 binder. The unused component is no longer exposed (the binder preserves it on write), both rows grey with SSAO off, and the single reset glyph restores the whole vector. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
Sun/moon and projector shadow bias only ever make sense at zero or below, and their offsets at zero or above; the sliders no longer allow values on the wrong side of zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
LLSliderCtrl auto-sizes its value box from log10(max_value) when text_width is not provided, which undercounts for maxima below 1 and collapses entirely for the bias sliders whose max is now 0, truncating values like -0.0020. Every slider with sub-1.0 fine values (shadow bias/offset pairs, SSR ray step and biases, flare radii and occlusion, grade black/white point) now carries text_width="56". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
LLPresetsManager grows a third "looks" subdirectory alongside graphic and camera: its own control watchers (loud when a whitelist key stops existing), PresetLooksActive/PresetLooksLastApplied dirty tracking, list-change signal, and save/delete handling. The ~105-key whitelist in getLooksControlNames is the single source of truth for what a Look carries - aesthetic settings from the Look and Lens tabs only, so a shared Look can never retune scene quality or performance. Applying a Look goes through loadLooksPreset, which writes only whitelisted keys parsed from the file rather than a raw loadFromFile, closing the foreign-key hole the camera presets needed the SL-20277 band-aid for. The Lightbox topbar becomes the Looks bar: apply combo, Save (falls through to Save As when nothing is active), Save As, Delete, Revert, and a modified indicator driven by the active-name blanking. The existing save/delete preset dialogs serve the new subdirectory with per-subdirectory titles and looks-signal subscriptions. Three starter Looks (Neutral, Soft Film, Golden Hour) ship as full whitelist snapshots in app_settings/looks, seeded into the user presets directory on first run following the camera-template pattern, and packaged next to colorlut in the manifest. The non-neutral pair are conservative first drafts pending in-viewer tuning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
The dialog prefills its name combo with existing preset names, and the combo text entry autocompletes typed prefixes to existing items - so typing a new Look name could silently commit to an existing one and overwrite it (observed against a bundled starter). For the looks subdirectory the dialog now opens with an empty, list-free name field and skips list-change repopulation, so typed names save verbatim; deliberate overwriting is what the Lightbox bar Save button is for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
RenderColorGrade is not a LUT toggle: the COLOR_GRADE shader permutation carries the entire grading chain, so Basic Grade, White Balance, Split Toning, Lift/Gamma/Gain, and Tone Curve all no-op while it is off (with it on and no LUT selected, the LUT path is disabled by zeroing its strength, so the combination is safe). The section is now titled Color Grading, the checkbox says "Enable color grading", and every grading row across all five sections greys against it so the dependency is visible instead of a silent no-op. The Soft Film and Golden Hour starters now switch grading on - as shipped they set grade values that the master toggle left inert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
doc/LIGHTBOX.md is the maintainer guide for exposing new effects: the declarative architecture and file map, copy-paste row templates for every row type, the accordion height rules (tab = panel + 29, the 26px slider+reset row pitch), gating patterns including the RenderColorGrade master switch, the sub-1.0 slider text_width requirement, cadence tooltip conventions, the Looks whitelist contract, and the developer-build staging trap. ARCHITECTURE.md and the Look panel header point to it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DeuFfMeQj2RNdoJ5bxfHXr
…rence still Flattens 51 commits from four branches: polish, colourgrade UI, grading fixes, and scopes v2. New GL-free models (colour wheel, curve, scope data, white-balance solver, grade history) with the widgets and pipeline as the only parts touching GL. Applied on top of the Lightbox v1 foundation, cherry-picked from rye-renderfun's rye/lightbox-v2. The published patch targeted 74cc0ae, which is not in this repo's history; CMakeLists.txt was hand-merged because its context named features this fork does not carry. Build: 0 warnings. ctest 131/131. Ratified in world: wheels, band graph, eyedropper, hold-to-compare, vectorscope, waveform/parade, per-section Compare, cursor readout, undo/redo, reference still + wipe, icon top bar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The scopes answer different questions about the same frame -- a histogram says how much of it sits at a level, a waveform says where, a vectorscope says what colour -- so the useful thing is to see several together rather than to cycle between them one at a time. The plot area now divides into one, two or four panes. AlchemyScopeLayout picks the arrangement and AlchemyScopePane0..3 say what each pane holds; right-clicking a pane sets its own. computePaneRects is the only place that divides the area, and it returns rects in the floater's coordinate space -- what draw() paints in and what handleRightMouseDown is given -- so the rect that drew a pane is the rect that hit-tests it. The three plot functions take (mode, rect) rather than reading the mode and the panel themselves. This costs nothing to measure. accumulate() already filled every channel, the chroma grid and the waveform grid on each capture whatever was on screen, so a fourth pane adds drawing and nothing else. Drawing is lopsided though: a waveform is WAVE_COLUMNS x WAVE_LEVELS cells per channel against a histogram's 256 bins, which is why four is a ceiling rather than a step. Defaults are the old behaviour exactly -- one pane, RGB histogram -- so upgrading changes nothing until the layout is switched. Spawning the menu needed LLContextMenu::show rather than the LLMenuGL::showPopup the neighbouring code uses. LLContextMenu overrides setVisible to ignore anything but false, and showPopup's only attempt to reveal a menu is setVisible(true), so it silently did nothing: the menu loaded, parented, populated and resolved its callbacks, and never appeared. show() takes screen coordinates where a mouse handler is given local ones. Both traps are now in doc/LIGHTBOX.md, which is also where the rule that a new scope is three edits and not one now lives. Build: 0 warnings. ctest 131/131. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grading and measuring belong together, and the only way to reach the scopes was the Advanced menu. A bar-graph icon at the end of the top bar opens them instead, set apart by the same twelve pixels that separate the Look buttons from the history pair, because it is a third kind of thing again: those act on this floater, this opens another window. Command_Stats_Icon is the viewer's existing Statistics glyph, already 18px like every other overlay in that bar, and it reads as what the window opens with -- the first pane is a histogram. The bar ends at 342px of the 412 it has at min_width, so it still fits with room to spare. Floater.Toggle, not Floater.ToggleOrBringToFront. The latter is written for toolbar buttons: it closes its target only after falling through `else if (!instance->isFrontmost())`, and pressing a button inside a floater makes that floater frontmost, so from here the close branch is unreachable and the button could only ever open and raise. Both are global commit callbacks in llui.cpp, so none of this needs C++. Also records why a rebuild does not restage an XUI-only edit: the copy is a POST_BUILD command on the viewer binary, so with no C++ changed nothing relinks and nothing is copied. That one cost a debugging round here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An accordion_tab can now carry a header_check_box: an interactive checkbox at the right end of its header, for a section that can be switched off without being collapsed. It is a full check_box params block, so control_name, enabled_control, tool_tip and commit_callback all behave as they do anywhere else. Omitting it leaves no checkbox at all rather than a hidden one, and every path that touches it is guarded on the pointer, so the twenty-odd accordions already in the viewer are untouched. Two things in LLAccordionCtrlTab stand in the way of anything living in a header, and both are now excepted rather than changed. handleMouseDown claims the whole header band for expand/collapse before offering the press to a child, so a control there is unreachable; handleToolTip does the same and forwards to the header without converting coordinates, so a header child's tooltip is never found once the tab is expanded. The exemptions ask first, and only for the checkbox's own rect. The mouse-down one falls through when the press is refused, because LLCheckBoxCtrl hit-tests against its bounding rect -- the box and label, not the full control -- and a hard return would leave a dead ring of pixels around the box. The five bypass checkboxes move out of the Color Grading section and onto Basic, Primaries, Split Toning, 3D LUT and Tone Curve. Ticked is the section switched on, which is the only way a box beside a section title reads, so onToggleSection inverts before setting a bit that suppresses. They gain enabled_control="RenderColorGrade", which the old row never had, and still carry no control_name: every grading setting is on the Looks whitelist, so a comparison built out of one would dirty the active Look. The doc had argued for keeping them together, on the grounds that A/B work means flipping between them and hunting through collapsed accordions is worse. That was wrong on its own terms: a collapsed accordion still shows its header, so a header checkbox is visible in every state a section has, all five line up when the sections are shut, and each one is now beside the controls it suppresses instead of a scroll away. Also corrects what this file says about the staging trap. A build does not stage XUI unreliably, it never stages it: the manifest's skins block is behind is_packaging_viewer(), which is false for --actions=copy, so the relink has nothing to do with it. That mistake cost a debugging round each of the three times it was believed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eader Basic - Advanced held two sliders, Brightness and Hue shift, which is what this file's own rule already calls a fold rather than a split: "fold instead of splitting when the advanced tail is small (~2-3 rows)". They move into Basic ahead of its Reset All and the sibling tab goes. Nothing in C++ needed changing -- onClickResetSection looks for sec_<id>_adv and simply does not find one -- and Exposure & Tone keeps its own Advanced sibling, so the convention stands where it earns its keep. The Color Grading section had a worse problem than sparseness. It held the master switch for the entire grading suite and the reference still, and it was closed by default, so the two things you most want at hand were the two things you had to go and open a section to reach. RenderColorGrade now rides that accordion's own header as a header_check_box, which is legible and throwable whether or not the section is open, and reads the same way as the five section switches below it. What tells the two kinds apart is worth knowing: the master carries control_name because it is a setting, and the section switches carry none because they are a viewing state that must not dirty the active Look. With the master gone from the body, the section opens by default, and what opens is now worth the space it takes. Two paragraphs of prose became one two-line note; the linear-light explanation they carried is still there, in its tooltip, where you go when you want to look it up rather than every time you glance at the tab. The section went from 218 to 140, so it now costs less open than it used to cost shut and then opened. Moving the master off the panel would have quietly broken that section's Reset All, since collectBoundControls walks the panel and the header is not in it. onClickResetSection now also checks atab_<section>'s header checkbox, walking the control rather than reading it directly, because LLCheckBoxCtrl hands control_name down to its button and the binding sits on the child. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guide was written when v2 landed and has drifted since, in the way a document drifts when the code around it keeps moving: not wrong in outline, wrong in the details you would only find by checking. Three of those were arithmetic. "Three widgets exist beyond the standard rows" was never true -- there are two tags, and the third richer control is the header checkbox, which is a param. "Three controls act on something other than a setting" was true until the reference still made it four. And the two closing paragraphs of the per-section bypass bullet had been stranded under the reference-still bullet that was inserted between them, so a rule about which tab carries the checkbox appeared to be a rule about reference stills. The claim that around twenty other accordions exist was out by an order of magnitude: 26 files in the English skin declare 75 of them, which makes the case for an additive param rather than weakens it. The Verify section still told you to tick a bypass to switch a section off, which is backwards since the switches moved to the headers. It now says untick, and adds the two checks a header control needs: that clicking it does not expand the section, and that its tooltip appears when the section is expanded -- the second only fails in that state, so testing collapsed proves nothing. What was missing was more of it than what was wrong. The widget reference listed the params it happened to need and not the ones a reader would look up, so both widgets now carry their full set. gl_polyline_2d and gl_polyfill_2d were written for this work, are the reason the graphs and scopes look the way they do, and appeared nowhere. The top bar had never been described at all despite being the one part of the floater with a fixed width budget, so it gets a section: the budget, the grouping gap that carries its meaning, why the buttons are icons, and where to get overlays. The scopes floater's own XUI gets a paragraph, since almost none of that window is widgets and that is worth saying out loud. An inventory table up front now says what v2 added and what it altered, and the architecture table gained the four files that were missing from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y effects The Lightbox could grade the light but not touch it, which left the first thing a photographer does outside the tool: pin the sky, so the thing being graded is one light rather than a moving one. Freezing does not pause anything, because there is nothing to pause. DayInstance::getProgress computes the cycle position from LLDate::now every frame, so the only way to stop the motion is to sample the running cycle at one position and install the result as a fixed local environment. That is what @setenv_daytime and the day cycle editor's timeline already do; this samples the water track as well as the sky, since a frozen sky over a moving sea is not frozen. Four things about changing LLEnvironment from a floater, none of them obvious and all of them now in the guide. @setenv is enforced inside LLEnvironment rather than by callers, so an unchecked control looks live and silently does nothing under restriction; there is no enable_callback on ordinary widgets, so the rows are greyed from the same draw() poll that reads their state back. Freezing covers up ENV_LOCAL, which is where a Personal Lighting sky lives, so what was there is captured and put back when Freeze is unticked -- clearing without capturing loses someone's sky silently. Reverting invalidates every reflection probe the old sky lit. And the frozen state is read from the world rather than mirrored in a member, which keeps the tab honest when the World menu changes the environment underneath it and is what lets scrubbing survive a close and reopen. The presets are not fractions of the cycle. Nothing in this viewer maps a cycle position to a clock -- the day cycle editor labels its timeline as a percentage, and a region puts its keyframes where it likes. So ALDayCycleLandmarks samples the cycle, reads the sun's height above the horizon, and takes noon and midnight from the extremes and sunrise and sunset from the crossings, interpolated so the answer beats the sample grid. A cycle missing a landmark greys that button rather than inventing one: a sun that never sets has a noon and no sunrise. It takes a sampler rather than a day cycle, so the arithmetic is tested against a sine wave with no viewer in the way, including the phase-shifted case a hardcoded 0.5 fails. Worth knowing why that case matters: LLSettingsSky::defaults caches its position-dependent result in a static, so the viewer's own default day cycle is eight identical frames and would make a wrong implementation look right. Sky Effects exposes three effects that were drawn entirely on the client and reachable only from Debug Settings: the aurora, the meteor streaks and the star field. One section rather than three, because two of them are a single control each and a section per slider reads as filing. The star count is a dropdown and not a slider, by this file's own rule: committing it regenerates every star position and rebuilds the vertex buffer, and a slider commits on every mouse-move. None of the three joins the Looks whitelist; a Look is the Look and Lens tabs, and one that switched the aurora on would be a surprise. Also records two layout rules that only fail on screen. A row of buttons has to be sized for min_width and not the default -- 361px, not 432 -- which the preset row learned by losing its last button the moment the floater was narrowed. And a one-line text wants height="16": an overflowing one does not clip or scroll, it draws over the row beneath. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Which order the renderer applies the grading sections in, and that White Balance and Lift / Gamma / Gain act in linear light while everything from Basic down acts on display values, is a maintainer's question. It is in doc/LIGHTBOX.md, where it is useful to someone adding a section; in the floater it was two lines of theory in front of a person trying to grade a picture. The order of the sections down the accordion still says it for anyone who cares to notice. What is left is the one line saying the header checkboxes exist, which is not an explanation but discoverability: the switch is a small box on a header that is easy to look straight past, and a control nobody finds may as well not be there. Its tooltip keeps the part a grader needs, that switching a section off saves nothing and does not dirty the Look. Renamed to grading_compare_note, since grading_order_note described what it no longer says. The section is the only one in the tab that opens by default, so the 14px comes back on every look at it: 140 to 126. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two fixes from the lightbox-v2 branch review: - captureScopeSample reallocated the sample target when the window's aspect or AlchemyScopeSampleWidth changed, but kept the two pixel-pack buffers at their first-frame size. A grown sample then had its glReadPixels refused (GL_INVALID_OPERATION against the too-small store) and the next collect memcpy'd past the end of the mapping. The buffers are now dropped and rebuilt alongside the target. - The day slider went through applyDayPosition, which skips the pre-freeze capture freezeSkyAt does. Scrubbing while a fixed sky someone else installed (Personal Lighting, say) was live overwrote it uncaptured, so unticking Freeze dropped to the region default instead of giving the sky back. The slider now takes the same freezeSkyAt path as the preset buttons; once the freeze is ours the capture short-circuits, so drags cost nothing extra per tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lder The last two findings from the branch review, plus the gap the second one made visible: - ALGradeHistory's group path skipped the MAX_DEPTH eviction, because erasing the front mid-group would shift mGroupIndex out from under the transaction still accumulating. endGroup now evicts once the outermost group closes and the index is dead, bringing the cursor down with the stack. A session of nothing but Look applies and section resets stays bounded like any other; test 14 is the group twin of the plain path's cap test. - The LUT combo listed every directory entry, so a readme or a subfolder in colorlut/ became a selectable entry that failed at apply time with only a log line to say why. It now takes regular files with the extensions setupGradingLUT actually loads, iterates with the non-throwing increment, and withholds the user-directory separator when nothing under it survives the filter. - With junk filtered out there was still no way to reach the user's colorlut folder from the viewer at all. The 3D LUT section grows an Open Folder button beside Reset All: creates the folder on first use, then opens it with the same cross-platform helper the poser's preset folder uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Brings in the resync with the Alchemy remote: the mesh repository threading work and the legacy-material shading corrections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request adds a Lightbox post-processing workflow with Looks presets, interactive color and curve controls, scopes, reference stills, white-balance tools, grading history, expanded rendering controls, bundled profiles, and unit tests. ChangesLightbox post-processing system
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Lightbox as ALFloaterLightBox
participant Pipeline as LLPipeline
participant ScopeData as ALScopeData
participant Scopes as ALFloaterScopes
Lightbox->>Pipeline: request scope capture
Pipeline->>ScopeData: accumulate and blend sampled frame
Scopes->>Pipeline: read scope data
Pipeline-->>Scopes: histogram, waveform, and vectorscope data
Scopes-->>Lightbox: display scope floater
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
indra/newview/aldaycyclelandmarks.h (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove one of the two include guards.
The file uses
#pragma onceand a macro guard.alcurvemodel.handalcurveeditorctrl.hin the same change use only the macro guard. Keep one mechanism for consistency.♻️ Proposed change
-#pragma once - `#ifndef` AL_DAYCYCLELANDMARKS_H `#define` AL_DAYCYCLELANDMARKS_H🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/aldaycyclelandmarks.h` around lines 25 - 28, Remove the redundant `#pragma` once from aldaycyclelandmarks.h and retain the AL_DAYCYCLELANDS_H macro include guard, matching the guard style used by alcurvemodel.h and alcurveeditorctrl.h.indra/newview/llpresetsmanager.cpp (1)
283-302: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMatch the null guard used by the sibling branches.
The
PRESETS_CAMERAandPRESETS_GRAPHICbranches checkcntrl_ptr.isNull()before they callgetCommitSignal(). The newPRESETS_LOOKSbranch callsgetCommitSignal()directly aftercontrolExists(). IfcontrolExists()andgetControl()ever disagree, this dereferences a null pointer. Add the same guard for consistency.♻️ Proposed guard
if (gSavedSettings.controlExists(ctrl_name)) { LLPointer<LLControlVariable> cntrl_ptr = gSavedSettings.getControl(ctrl_name); - mLooksChangedSignals.push_back(cntrl_ptr->getCommitSignal()->connect(boost::bind(&LLPresetsManager::looksSettingChanged, this))); + if (cntrl_ptr.isNull()) + { + LL_WARNS("Presets") << "Unable to set signal on global setting '" << ctrl_name << "'" << LL_ENDL; + } + else + { + mLooksChangedSignals.push_back(cntrl_ptr->getCommitSignal()->connect(boost::bind(&LLPresetsManager::looksSettingChanged, this))); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/llpresetsmanager.cpp` around lines 283 - 302, Update the PRESETS_LOOKS handling in the PresetsManager branch to check cntrl_ptr.isNull() before calling getCommitSignal(), matching the guards in the PRESETS_CAMERA and PRESETS_GRAPHIC branches. Only connect looksSettingChanged when the control pointer is valid, while preserving the existing missing-control warning behavior.indra/newview/llpresetsmanager.h (1)
85-89: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider building the Looks control list once.
getLooksControlNamesconstructs a fixed vector of about 110std::stringobjects on every call. The comment states that the Lightbox undo stack also reads this list, so the call frequency may be higher than the three call sites in this file. Store the list in a function-localstatic const std::vector<std::string>in the definition and copy from it, or return aconst&.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/llpresetsmanager.h` around lines 85 - 89, Update getLooksControlNames to initialize the fixed control-name list once using a function-local static const vector, then reuse it on subsequent calls by copying into the output parameter or returning a const reference while preserving the existing public API contract.indra/newview/lutcube.h (1)
54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parsedoes not reset parser state, so it is only safe once per object.The documentation presents
parseas the public entry point for exercising the parser without the filesystem.parsedoes not clearcolorCube,size,currentX,currentY,currentZ,entryCount,sawSize, orfailed. A second call on the sameLutCubefails at the duplicateLUT_3D_SIZEcheck and keeps the earlierfailedstate. If callers or tests are expected to reuse an instance, reset the state at the start ofparse.♻️ Proposed change in `LutCube::parse` (indra/newview/lutcube.cpp)
void LutCube::parse(std::istream& stream) { + // Start from a clean slate so an instance can be parsed into more than once. + colorCube.clear(); + size = 0; + currentX = currentY = currentZ = 0; + entryCount = 0; + sawSize = false; + failed = false; + std::string line;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/lutcube.h` around lines 54 - 62, Reset all parser state at the start of LutCube::parse, including colorCube, size, currentX, currentY, currentZ, entryCount, sawSize, and failed, so each invocation parses the provided stream independently and does not retain duplicate-header or failure state from earlier calls.indra/newview/pipeline.h (1)
796-807: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a reset path for the grading bypass state.
The documentation places the duty to clear a bit on the caller that sets it, and names
~ALFloaterLightBox. That invariant has no enforcement. If the destructor does not run, or a future caller sets a bit and returns early,sGradeBypassMaskkeeps part of the grade suppressed for the rest of the session. No control in the interface shows that state, so the result looks like a rendering defect rather than a stuck flag.sGradeBypasshas the same exposure if a key-up event is lost.Consider clearing both in a pipeline reset point, for example
LLPipeline::cleanuporreleaseGLBuffers, so the state cannot outlive the feature that set it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/pipeline.h` around lines 796 - 807, Add a pipeline reset path that clears both LLPipeline::sGradeBypassMask and sGradeBypass, preferably in LLPipeline::cleanup or releaseGLBuffers. Ensure this reset runs when rendering state is reinitialized so bypass flags cannot persist after their owning feature is gone.indra/newview/alscopedata.cpp (1)
253-272: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake the waveform blend robust to an empty grid.
The equal-size branch indexes
mWaveforCH_COUNT * WAVE_COLUMNS * WAVE_LEVELSelements. If both grids are empty, the sizes match and the loop reads and writes out of bounds. TheisEmpty()guards above make this unreachable today, because a positivemSampleCountimplies a filled grid. Add the emptiness test to the mismatch condition so a later change toaccumulatecannot turn this into a heap overflow.🛡️ Proposed guard
- if (mWave.size() != other.mWave.size()) + if (mWave.empty() || mWave.size() != other.mWave.size())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/alscopedata.cpp` around lines 253 - 272, Update the assignment/blend logic around the mWave size comparison so the replacement branch also runs when mWave is empty, by adding the appropriate mWave emptiness check to the mismatch condition. Preserve the existing copy and peak-copy behavior for size differences, while ensuring the equal-size blending loop is only reached for non-empty grids.indra/newview/alfloaterlightbox.cpp (1)
1163-1191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
ScopedTruefor the re-entry guard.The file already defines
ScopedTruefor exactly this pattern. A manual set/clear pair leavesmToneCurveUpdatingstuck attrueon any early return added later, which disables the graph refresh permanently. The same applies tomSplitToneUpdatinginonCommitSplitToneGraphand tomVec3UpdatinginrefreshVec3Row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/alfloaterlightbox.cpp` around lines 1163 - 1191, Replace the manual mToneCurveUpdating set/clear guard in the tone-curve update handler with the existing ScopedTrue pattern so the flag is restored on every exit path. Apply the same ScopedTrue-based guard to mSplitToneUpdating in onCommitSplitToneGraph and mVec3Updating in refreshVec3Row, preserving their existing update behavior.indra/newview/skins/default/xui/en/panel_lightbox_look.xml (1)
536-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
lut_enableto match what it controls.This header checkbox is the master switch for all color grading and binds to
RenderColorGrade. The namelut_enabledescribes only the 3D LUT. Thesec_lutsection already has its own checkbox namedsection_lut, so the two names invite confusion in XUI and in anygetChildlookup. A name such asgrading_enablestates the scope correctly.♻️ Proposed rename
<accordion_tab.header_check_box - name="lut_enable" + name="grading_enable" control_name="RenderColorGrade"Update any matching
getChildcall inalfloaterlightbox.cpp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/skins/default/xui/en/panel_lightbox_look.xml` around lines 536 - 539, Rename the master checkbox identifier from lut_enable to grading_enable in the accordion_tab header, and update the corresponding getChild lookup in alfoaterlightbox.cpp to use the new name. Leave the section_lut identifier unchanged.
🔇 Additional comments (117)
doc/ARCHITECTURE.md (1)
117-118: LGTM!doc/LIGHTBOX.md (3)
114-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Clarify the source of row ranges and tooltips.
This section says the settings
Commentsuppliesmin_val,max_val, and the tooltip. The row template sets all three values directly in XUI. If no binding copiesCommentinto those parameters, the instructions are incomplete. State which metadata is automatic and which must be repeated in XUI.
482-487: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Document the implementation work for a new scope mode.
The guide calls a new
EModea three-edit change. A new visualization may also requireALScopeDataaccumulation and draw dispatch. State that the three edits apply only when reusing an existing renderer, or document the required implementation changes.
913-922: 📐 Maintainability & Code Quality | 🔵 Trivial
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the developer-build staging claim.
This section states that developer builds never stage XUI and that
--actions=copyexcludes skins. The referenced build files are not included here. Confirm the actualPOST_BUILDarguments andis_packaging_viewer()behavior before keeping this absolute instruction.indra/newview/CMakeLists.txt (1)
160-164: LGTM!Also applies to: 180-185, 213-216, 939-943, 959-964, 992-995, 2396-2402, 2413-2413, 2425-2434
indra/newview/alcolorwheelctrl.cpp (9)
45-98: LGTM!
100-204: LGTM!
206-243: LGTM!
245-291: LGTM!
293-330: LGTM!
332-388: LGTM!
406-493: LGTM!
517-562: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the new llrender2dutils signatures used here.
This file calls
gl_washer_angular_2dwith a colour vector andgl_polyline_2dwith a four-argument form. Both come from the rendering-primitives layer, which is not in this review context. Confirm the declarations match these call sites, including the closed-polyline argument and the colour-per-step count expected by the washer.
495-516: LGTM!Also applies to: 563-566
indra/newview/alcolorwheelctrl.h (1)
26-166: LGTM!indra/newview/alcolorwheelmodel.cpp (1)
33-191: LGTM!indra/newview/alcolorwheelmodel.h (1)
25-163: LGTM!indra/newview/alcurveeditorctrl.cpp (2)
46-148: LGTM!
173-334: LGTM!indra/newview/tests/aldaycyclelandmarks_test.cpp (1)
15-163: LGTM!indra/newview/tests/alwhitebalancesolver_test.cpp (1)
23-229: LGTM!indra/newview/tests/lutcube_test.cpp (1)
26-292: LGTM!indra/newview/alcurveeditorctrl.h (2)
153-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Consider default member initializers for the scalar parameters.
mGridDivisions,mCurveSamples,mHandleRadius,mCurveWidth, andmDrawDiagonalhave no in-class initializers, unlikemDragIndexand the grab offsets. The constructor is inalcurveeditorctrl.cpp, which is not part of this context. If any member is not assigned there,draw()reads an indeterminate value. Default initializers remove that risk and keep the block consistent.♻️ Proposed change
- S32 mGridDivisions; - S32 mCurveSamples; - S32 mHandleRadius; - F32 mCurveWidth; - bool mDrawDiagonal; + S32 mGridDivisions = 4; + S32 mCurveSamples = 64; + S32 mHandleRadius = 4; + F32 mCurveWidth = 1.f; + bool mDrawDiagonal = true;Run the following script to confirm the constructor assigns every member:
94-122: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify drag state recovery when the control loses mouse capture.
handleMouseUpis the only declared path that can clearmDragIndex.LLUICtrlalso deliversonMouseCaptureLost()when capture is taken away, for example when the floater closes during a drag. If the implementation sets mouse capture inhandleMouseDownand does not overrideonMouseCaptureLost,mDragIndexstays set andgetActiveHandle()reports a stale index.Run the following script to check the implementation:
indra/newview/alcurvemodel.cpp (1)
35-82: LGTM!Also applies to: 91-212, 214-339
indra/newview/alcurvemodel.h (1)
54-170: LGTM!indra/newview/aldaycyclelandmarks.cpp (2)
25-28: 📐 Maintainability & Code Quality | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the first include matches the viewer's precompiled-header convention.
This translation unit includes only its own header.
alcurvemodel.cppin the same cohort starts withlinden_common.h. Files underindra/newview/are normally compiled with a precompiled header, and the compiler requires that header as the first include when PCH is enabled. If the build enables PCH for this source file, compilation fails or the file is silently excluded from PCH.Run the following script to check the convention and the build settings:
44-127: LGTM!indra/newview/tests/alcolorwheelmodel_test.cpp (1)
23-392: LGTM!indra/newview/tests/alcurvemodel_test.cpp (1)
24-484: LGTM!indra/newview/tests/algradehistory_test.cpp (1)
24-285: LGTM!indra/newview/tests/alscopedata_test.cpp (2)
365-368: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the out-of-range
EChannelcasts are well defined.
(ALScopeData::EChannel)99and(ALScopeData::EChannel)-3are only defined behaviour ifEChannelhas a fixed underlying type. For an unscoped enumeration without a fixed underlying type, a value outside the enumeration's value range produces undefined behaviour, and a sanitizer build can report it. IfEChannelhas no fixed underlying type, add one, or test the guard withCH_COUNT, as line 640 already does.Run the following script to check the declaration:
25-322: LGTM!Also applies to: 327-364, 369-647
indra/newview/app_settings/looks/Golden%20Hour.xml (1)
1-1228: LGTM!indra/newview/app_settings/looks/Neutral.xml (1)
1-1228: LGTM!indra/newview/app_settings/looks/Soft%20Film.xml (1)
1-1228: LGTM!indra/newview/app_settings/settings.xml (1)
10044-10093: LGTM!indra/newview/app_settings/settings_alchemy.xml (1)
1198-1296: LGTM!Also applies to: 1590-1611, 2020-2020, 2246-2267
indra/newview/llcolorswatch.cpp (1)
304-321: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the Color3 conversion and control-type symbols.
The change depends on two symbols from the newly added headers. Confirm that
LLColor3provides a constructor fromLLColor4inindra/llmath/v3color.h, and thatTYPE_COL3andLLControlVariable::type()are declared inindra/llcommon/llcontrol.h. Also confirm that no remaining caller reachesLLColorSwatchCtrl::set()withfrom_event == falsefor aTYPE_COL3-bound swatch, because that path still writes a four-component value throughsetControlValue.Also applies to: 325-354
indra/newview/llfloaterdeleteprefpreset.cpp (1)
55-55: LGTM!indra/newview/llfloatersaveprefpreset.cpp (2)
163-189: 🗄️ Data Integrity & Integration | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the button order of the
LightBoxLookOverwritenotification.The callback treats option index
0as confirm and any other index as cancel. Ifnotifications.xmldeclares the Cancel button first, the logic inverts: a confirm click does nothing, and a cancel click overwrites the existing Look. That inversion destroys a saved Look without consent. Confirm that the affirmative button is declared first, or compare against the button name instead of the index.
112-138: 🎯 Functional Correctness | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify empty-name handling and the per-subtype title strings for Looks.
onOpennow clears the combo for Looks, so the name field starts empty.onBtnSavereadsmPresetCombo->getSimple()without an emptiness check. Confirm thatonPresetNameEditeddisablesmSaveButtonwhen the text is empty; otherwise a click saves a Look named"". Also confirm thatfloater_save_pref_preset.xmldeclares atitle_looksstring, becausehasStringfalls back silently to the default title when the string is absent.indra/newview/llfloatersettingsdebug.cpp (1)
477-484: LGTM!indra/newview/llpresetsmanager.cpp (4)
516-519: 🔒 Security & Privacy | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify how
RenderColorGradeLUTis consumed before a Look sets it.
loadLooksPresetwritesRenderColorGradeLUTfrom the Look file without validation. Look files are shared between users, so this string is untrusted input that names a file the viewer then opens. Confirm that the LUT loader resolves the value inside a fixed LUT directory and rejects path separators and..segments. If it concatenates the value onto a base path without checks, a crafted Look reads arbitrary files from the recipient's disk.
141-220: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the
LLSD::insertandLLFile::copysignatures.
copyDefaultLookscallsseeded.insert(file, true)andLLFile::copy(src, dst)and treats the copy result as a bool. Confirm thatLLSD::insertaccepts aStringkey with anLLSDvalue in this tree, and thatLLFile::copyis a static member with that two-argument form and a bool return. IfLLFile::copyreturns an int status instead, the failure branch inverts and a failed seed gets recorded as successful, which permanently skips that bundled Look.
890-949: LGTM!
46-54: LGTM!Also applies to: 56-76, 88-92, 318-405, 418-427, 622-671, 788-795, 994-1006, 1056-1060
indra/newview/llpresetsmanager.h (1)
40-45: LGTM!Also applies to: 107-114, 123-128
indra/newview/viewer_manifest.py (1)
107-109: LGTM!indra/llrender/llrender2dutils.cpp (3)
269-387: LGTM!
389-422: LGTM!
1224-1262: LGTM!indra/llrender/llrender2dutils.h (1)
37-39: LGTM!Also applies to: 53-80, 104-118
indra/llrender/llshadermgr.cpp (1)
1939-1943: LGTM!Also applies to: 1969-1980
indra/llrender/llshadermgr.h (1)
517-521: LGTM!indra/llui/llaccordionctrltab.cpp (6)
49-61: LGTM!
93-98: LGTM!Also applies to: 110-110, 219-227
280-312: LGTM!
413-413: LGTM!Also applies to: 436-436, 449-478
618-651: LGTM!
1265-1279: LGTM!indra/llui/llaccordionctrltab.h (1)
32-32: LGTM!Also applies to: 85-97, 151-156, 242-254
indra/newview/llviewerfloaterreg.cpp (1)
48-48: LGTM!Also applies to: 599-599
indra/newview/llviewerinput.cpp (1)
975-993: LGTM!Also applies to: 1108-1111
indra/newview/lutcube.cpp (1)
263-274: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that
llclampis reachable from this translation unit.
clampTripelnow callsllclamp. The includes arelinden_common.h,lutcube.h,llfile.h,<cmath>,<istream>, and<sstream>.llclampis declared inllmath.h. If the current include set only reaches it transitively, a future header change breaks this file. Add the direct include if the dependency is indirect.indra/newview/pipeline.cpp (2)
8110-8117: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the
ALWhiteBalanceSolver::gaincontract.
colorCorrectnow depends onALWhiteBalanceSolver::gain(F32, F32)returning anLLVector3of linear-sRGB gains, and uploadswb_gain.mVdirectly as three floats. That class is defined in another layer of this stack and is not in this prompt. Confirm the signature, the return type, and that the clamping and Duv-to-uv scale the comment describes live in the solver, so the eyedropper inverts the same function.
323-325: LGTM!Also applies to: 1309-1310, 1606-1636, 7298-7360, 7362-7399, 7428-7478, 7754-7766, 7822-7829, 7843-7843, 7878-7880, 8075-8090, 8138-8140, 8168-8170, 8194-8202, 8222-8222, 9137-9143, 9248-9256, 9273-9283, 9295-9295, 9316-9317, 9326-9326, 9342-9358
indra/newview/pipeline.h (1)
48-48: LGTM!Also applies to: 156-223, 777-795, 872-895
indra/newview/alfloaterlightbox.cpp (12)
60-154: LGTM!
193-197: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Guard the picker singleton with
instanceExists().
ALToolScenePicker::getInstance()constructs the singleton if it does not exist yet. The destructor can run at viewer shutdown, and a first-time construction of anLLSingletonduring shutdown produces a singleton-lifetime error. Add an existence check before the comparison.♻️ Proposed guard
if (LLToolMgr::instanceExists() && + ALToolScenePicker::instanceExists() && LLToolMgr::getInstance()->getCurrentTool() == ALToolScenePicker::getInstance())
216-224: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the settings keys exist, or guard the pointers.
getControl("AlchemyRenderTonemapType")andgetControl("PresetLooksActive")returnnullptrfor an unknown key. The code dereferences the result at once, so a missing or renamed key crashes atpostBuild. Other call sites in this file checkgetControlfor null before use.
286-374: LGTM!
386-402: 🎯 Functional Correctness | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the section identifier contract between the XUI and both callbacks.
onClickResetSectiontreatsuserdataas a panel name and looks upfindChild<LLPanel>(section)plussection + "_adv"and"atab_" + section.onToggleSectiontreatsuserdataas one ofbasic,primaries,split,lut,curve. The comment at Line 431 states that Reset All walkssec_<id>andsec_<id>_adv, which does not match the code. If the XUI passes a bare id such asbasictoLightBox.ResetSection, thenfindChild<LLPanel>("basic")finds nothing and the reset silently does nothing.Also applies to: 429-446
467-551: LGTM!
553-865: LGTM!
867-965: LGTM!
1316-1345: 🎯 Functional Correctness | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify both notification names exist.
LLNotificationsUtil::addwith an unknown template name shows nothing to the user and only logs. The picker then appears to do nothing when the sample is too dark or unreachable. ConfirmLightBoxWhiteBalanceTooDarkandLightBoxWhiteBalanceUnreachableare declared innotifications.xml.
1193-1292: LGTM!
1347-1431: LGTM!
1433-1471: LGTM!indra/newview/alfloaterlightbox.h (1)
37-247: LGTM!indra/newview/alfloaterscopes.cpp (6)
69-128: LGTM!
184-248: LGTM!
250-493: LGTM!
495-543: 🎯 Functional Correctness | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify all floater string names exist in
floater_scopes.xml.
updateReadoutsreadswaiting,clipping_pixel, andclipping.drawPaneLabelreads the ninemode_*names returned bymodeStringName. A missing<string>entry makesgetStringlog a warning and return placeholder text that is drawn on the plot.Also applies to: 733-737
586-686: LGTM!Also applies to: 740-806
110-120: 🎯 Functional Correctness | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the scopes floater is registered as a single instance.
LLPipeline::sScopeCaptureis a global flag with no reference count. If two instances ofALFloaterScopescan exist, closing one stops sampling for the other, and the remaining floater draws stale data.indra/newview/alfloaterscopes.h (1)
70-181: LGTM!indra/newview/algradehistory.cpp (1)
31-180: LGTM!indra/newview/algradehistory.h (1)
60-125: LGTM!indra/newview/alscopedata.cpp (3)
34-69: LGTM!
71-206: LGTM!
277-314: LGTM!indra/newview/alscopedata.h (1)
56-185: LGTM!indra/newview/altoolscenepicker.cpp (2)
61-79: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
requestScenePixelrejects out-of-range coordinates.
handleMouseDowntakes the mouse capture, sohandleMouseUpruns even when the release lands outside the world view. The header states that a pick can be abandoned by dragging off, but this code samples at whateverx, ythe release reports, including negative values or values past the render target. ConfirmLLPipeline::requestScenePixelclamps or rejects such coordinates, or add the bounds test here.
33-59: LGTM!Also applies to: 81-85
indra/newview/altoolscenepicker.h (1)
50-72: LGTM!indra/newview/alwhitebalancesolver.cpp (3)
33-93: LGTM!
95-139: LGTM!
141-323: LGTM!indra/newview/alwhitebalancesolver.h (1)
51-122: LGTM!indra/newview/skins/default/textures/textures.xml (1)
999-999: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that the
script_save.pngasset exists.
floater_lightbox_settings.xmlreferencesScript_Saveas animage_overlay. If the PNG is missing, the button renders with no glyph.indra/newview/skins/default/xui/en/control_table_contents_media.xml (1)
86-95: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
grade_bypass_keyis registered as a keybinding action.The row
valuemust match an action name registered inllviewerinput.cppand present in the defaultkeys.xmlschema. If the name does not match, the row appears in Preferences but the binding never fires.indra/newview/skins/default/xui/en/floater_delete_pref_preset.xml (1)
14-14: LGTM!indra/newview/skins/default/xui/en/floater_lightbox_settings.xml (2)
185-189: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that
panel_lightbox_lens.xmlis included in this pull request.The tab container loads four panels.
panel_lightbox_look.xml,panel_lightbox_scene.xml, andpanel_lightbox_sky.xmlare added by this pull request.panel_lightbox_lens.xmldoes not appear in the changed file set or in the stack outline. If the file is absent, the Lens tab fails to build and the floater reports a missing-panel error at runtime.
165-167: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Two call sites depend on the
scopesfloater registration name. Both the Lightbox toolbar button and the viewer menu item resolve the literal stringscopesthroughLLFloaterReg. If the floater is registered under a different name, both entry points silently do nothing.
indra/newview/skins/default/xui/en/floater_lightbox_settings.xml#L165-L167: confirmFloater.Togglewithparameter="scopes"matches the registered name, and confirm the registration bindsfloater_scopes.xml.indra/newview/skins/default/xui/en/menu_viewer.xml#L864-L873: confirmFloater.VisibleandFloater.Toggleuse the same registered name so the checkmark tracks the floater state.indra/newview/skins/default/xui/en/floater_save_pref_preset.xml (1)
12-15: LGTM!indra/newview/skins/default/xui/en/floater_scopes.xml (2)
53-54: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that the two bound settings exist.
AlchemyScopeLayoutandAlchemyScopeLogScalemust be declared insettings_alchemy.xml. Acontrol_namethat does not resolve leaves the widget unbound and the saved layout is lost between sessions. The same applies to theAlchemyScopePane*settings named inmenu_scopes_pane.xml.Also applies to: 82-83
84-92: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm a keyboard path exists for choosing the scope in a pane.
scope_plotis a plain panel. The only documented way to change what a pane shows is a right-click context menu. If the panel does not take keyboard focus and does not handle a context-menu key, keyboard-only users cannot select a scope mode.indra/newview/skins/default/xui/en/menu_scopes_pane.xml (1)
9-73: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the parameter values against
ALFloaterScopes::EMode.The nine items hardcode the enum ordinals 0 through 8. The set is complete and has no duplicates, and the item names match the
mode_*strings infloater_scopes.xml. The mapping breaks silently if a future change reorders the enum.indra/newview/skins/default/xui/en/notifications.xml (1)
9311-9338: LGTM!indra/newview/skins/default/xui/en/panel_lightbox_look.xml (2)
1485-1502: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that the LUT loader sanitises the free-text value.
colorlut_combosetsallow_text_entry="true"and writes directly intoRenderColorGradeLUT. A user can type any string up to 200 characters, including../sequences or an absolute path. If the loader joins this value onto thecolorlutdirectory without normalising and confining the result, the setting becomes a path-traversal vector that reads arbitrary files from disk. The value is also persisted in a Look preset, so a shared Look can carry a crafted path.Confine the resolved path to the LUT directory, or reject any value containing a path separator.
1619-1647: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet an initial value on
tone_curve_channel.This combo has no
control_nameand novalueattribute. Unlessalfloaterlightbox.cppselects an item duringpostBuild, the combo renders empty on first open and the graph has no channel selected. Addvalue="-1"so the Linked item is selected from the start.🐛 Proposed fix
height="18" name="tone_curve_channel" + value="-1" tool_tip="Which channel the graph edits. Linked moves all three together; a single channel shows the other two behind it."> Likely an incorrect or invalid review comment.indra/newview/skins/default/xui/en/panel_lightbox_scene.xml (2)
1383-1400: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that
LightBox.CommitVec3preserves the unexposed third component.This section exposes only
vec3_RenderSSAOEffect_0andvec3_RenderSSAOEffect_1. Every other vector row in this feature exposes all three spinners, for examplevec3_RenderColorGradeCurveToe_0through_2inpanel_lightbox_look.xml. If the handler rebuilds the vector by reading the sibling spinners it finds, the missing_2spinner makes the third component read as zero and the setting is silently corrupted on every edit.The handler must read the current control value and replace only the indexed component.
Also applies to: 1426-1443
296-301: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
LightBox.ResetSectionexpands to the Advanced sibling.These three buttons pass only the base section id (
sec_reflect,sec_shadow,sec_perf) while their tooltips promise "including Advanced". The handler must also reset the controls insec_reflect_adv,sec_shadow_adv, andsec_perf_adv. If it does not, the tooltip is wrong and advanced settings survive a section reset.Also applies to: 1050-1055, 1623-1628
indra/newview/skins/default/xui/en/panel_lightbox_sky.xml (2)
311-329: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that the renderer clamps
RenderMeteorMaxCountto 64.The tooltip states that the meteor vertex buffer is sized once for 64 entries and is never reallocated. This slider caps at 64, but the setting is also reachable from Debug Settings and from a saved Look. If the renderer trusts the setting value without clamping, a value above 64 writes past the end of the buffer.
30-184: LGTM!
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/LIGHTBOX.md`:
- Around line 41-44: Add LUT parser coverage to the Lightbox documentation
inventory by including the lutcube component and lutcube_test alongside the
existing tested components. Update the wording or lists so maintainers clearly
recognize LUT arithmetic/parser validation as covered.
- Around line 762-771: Resolve the minimum-width inconsistency in the Lightbox
sizing guidance: reconcile the 412px value referenced earlier with the 420px
calculation, identify the authoritative minimum width, and recalculate the
usable inner width and button-row fit using that same value. Update the
surrounding arithmetic and stated endpoint so the documented narrowest case
accurately prevents clipping.
- Around line 563-566: The shader-checker documentation in LIGHTBOX.md names
check_glsl_decls.py and check_glsl_uniforms.py without actionable locations or
commands. Update the relevant instructions to include each script’s repository
path and exact invocation, or document a supported replacement, while preserving
the guidance to run checks after modifying shaders.
In `@indra/newview/alcurveeditorctrl.cpp`:
- Around line 150-171: Guard both pointer handlers with getEnabled() before
widget-specific actions: in indra/newview/alcurveeditorctrl.cpp lines 150-171,
have ALCurveEditorCtrl::handleMouseDown return LLUICtrl::handleMouseDown(x, y,
mask) when disabled, before hitTest or mouse capture; in
indra/newview/alcolorwheelctrl.cpp lines 390-404, have handleDoubleClick return
LLUICtrl::handleDoubleClick(x, y, mask) when disabled, before onReset().
In `@indra/newview/llviewerwindow.cpp`:
- Around line 5347-5348: Update the rawSnapshot call within saveSnapshot to pass
its type parameter positionally after show_balance and before max_size,
preserving the requested snapshot layer such as depth instead of relying on
rawSnapshot’s default.
In `@indra/newview/lutcube.cpp`:
- Around line 196-201: Update parseLine around splitTripel to initialize x, y,
and z and immediately return when splitTripel rejects the triple. Only call
clampTripel, writeColor, and increment entryCount for successfully parsed finite
values, while preserving the existing failed-state handling for the caller.
- Around line 66-70: Update MAX_LUT_SIZE and its adjacent allocation comment to
reflect colorCube’s 16-bit storage: 256³ × 4 channels uses 128 MiB, not 64 MiB.
Prefer lowering MAX_LUT_SIZE to 64 while retaining MIN_LUT_SIZE and accepting
authoring-tool sizes 17, 33, and 65 as intended.
In `@indra/newview/pipeline.cpp`:
- Around line 7508-7532: In the resolution-change block around mScopeSample
reallocation, clear both mScopeReadback and mScopeData alongside resetting
mScopePBOInFlight so getScopePixel cannot use captures from the previous
dimensions. Preserve the existing resize, allocation-failure, and in-flight
buffer handling.
In `@indra/newview/skins/default/xui/en/panel_lightbox_scene.xml`:
- Around line 574-577: Update the combo_box.item with name and value "1" so its
label is "Minimum (1)" instead of "None", accurately describing the enabled
reflection probe count and matching the other item labels.
In `@indra/newview/tests/alscopedata_test.cpp`:
- Around line 323-326: Update the test setup around ALScopeData::accumulate so
the flat buffer created for b uses ALScopeData::BIN_COUNT instead of the
hard-coded 256 pixel count. Keep the accumulate call and other channel values
unchanged.
---
Nitpick comments:
In `@indra/newview/aldaycyclelandmarks.h`:
- Around line 25-28: Remove the redundant `#pragma` once from
aldaycyclelandmarks.h and retain the AL_DAYCYCLELANDS_H macro include guard,
matching the guard style used by alcurvemodel.h and alcurveeditorctrl.h.
In `@indra/newview/alfloaterlightbox.cpp`:
- Around line 1163-1191: Replace the manual mToneCurveUpdating set/clear guard
in the tone-curve update handler with the existing ScopedTrue pattern so the
flag is restored on every exit path. Apply the same ScopedTrue-based guard to
mSplitToneUpdating in onCommitSplitToneGraph and mVec3Updating in
refreshVec3Row, preserving their existing update behavior.
In `@indra/newview/alscopedata.cpp`:
- Around line 253-272: Update the assignment/blend logic around the mWave size
comparison so the replacement branch also runs when mWave is empty, by adding
the appropriate mWave emptiness check to the mismatch condition. Preserve the
existing copy and peak-copy behavior for size differences, while ensuring the
equal-size blending loop is only reached for non-empty grids.
In `@indra/newview/llpresetsmanager.cpp`:
- Around line 283-302: Update the PRESETS_LOOKS handling in the PresetsManager
branch to check cntrl_ptr.isNull() before calling getCommitSignal(), matching
the guards in the PRESETS_CAMERA and PRESETS_GRAPHIC branches. Only connect
looksSettingChanged when the control pointer is valid, while preserving the
existing missing-control warning behavior.
In `@indra/newview/llpresetsmanager.h`:
- Around line 85-89: Update getLooksControlNames to initialize the fixed
control-name list once using a function-local static const vector, then reuse it
on subsequent calls by copying into the output parameter or returning a const
reference while preserving the existing public API contract.
In `@indra/newview/lutcube.h`:
- Around line 54-62: Reset all parser state at the start of LutCube::parse,
including colorCube, size, currentX, currentY, currentZ, entryCount, sawSize,
and failed, so each invocation parses the provided stream independently and does
not retain duplicate-header or failure state from earlier calls.
In `@indra/newview/pipeline.h`:
- Around line 796-807: Add a pipeline reset path that clears both
LLPipeline::sGradeBypassMask and sGradeBypass, preferably in LLPipeline::cleanup
or releaseGLBuffers. Ensure this reset runs when rendering state is
reinitialized so bypass flags cannot persist after their owning feature is gone.
In `@indra/newview/skins/default/xui/en/panel_lightbox_look.xml`:
- Around line 536-539: Rename the master checkbox identifier from lut_enable to
grading_enable in the accordion_tab header, and update the corresponding
getChild lookup in alfoaterlightbox.cpp to use the new name. Leave the
section_lut identifier unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69c69bdf-0b71-4733-a88b-fe57c1085f75
⛔ Files ignored due to path filters (3)
indra/newview/app_settings/shaders/class1/alchemy/blitWithEffectsF.glslis excluded by!**/*.glslindra/newview/app_settings/shaders/class1/alchemy/colorGradeUtilF.glslis excluded by!**/*.glslindra/newview/skins/default/textures/script_editor/script_save.pngis excluded by!**/*.png
📒 Files selected for processing (70)
doc/ARCHITECTURE.mddoc/LIGHTBOX.mdindra/llrender/llrender2dutils.cppindra/llrender/llrender2dutils.hindra/llrender/llshadermgr.cppindra/llrender/llshadermgr.hindra/llui/llaccordionctrltab.cppindra/llui/llaccordionctrltab.hindra/newview/CMakeLists.txtindra/newview/alcolorwheelctrl.cppindra/newview/alcolorwheelctrl.hindra/newview/alcolorwheelmodel.cppindra/newview/alcolorwheelmodel.hindra/newview/alcurveeditorctrl.cppindra/newview/alcurveeditorctrl.hindra/newview/alcurvemodel.cppindra/newview/alcurvemodel.hindra/newview/aldaycyclelandmarks.cppindra/newview/aldaycyclelandmarks.hindra/newview/alfloaterlightbox.cppindra/newview/alfloaterlightbox.hindra/newview/alfloaterscopes.cppindra/newview/alfloaterscopes.hindra/newview/algradehistory.cppindra/newview/algradehistory.hindra/newview/alscopedata.cppindra/newview/alscopedata.hindra/newview/altoolscenepicker.cppindra/newview/altoolscenepicker.hindra/newview/alwhitebalancesolver.cppindra/newview/alwhitebalancesolver.hindra/newview/app_settings/looks/Golden%20Hour.xmlindra/newview/app_settings/looks/Neutral.xmlindra/newview/app_settings/looks/Soft%20Film.xmlindra/newview/app_settings/settings.xmlindra/newview/app_settings/settings_alchemy.xmlindra/newview/llcolorswatch.cppindra/newview/llfloaterdeleteprefpreset.cppindra/newview/llfloatersaveprefpreset.cppindra/newview/llfloatersettingsdebug.cppindra/newview/llpresetsmanager.cppindra/newview/llpresetsmanager.hindra/newview/llviewerfloaterreg.cppindra/newview/llviewerinput.cppindra/newview/llviewerwindow.cppindra/newview/lutcube.cppindra/newview/lutcube.hindra/newview/pipeline.cppindra/newview/pipeline.hindra/newview/skins/default/textures/textures.xmlindra/newview/skins/default/xui/en/control_table_contents_media.xmlindra/newview/skins/default/xui/en/floater_delete_pref_preset.xmlindra/newview/skins/default/xui/en/floater_lightbox_settings.xmlindra/newview/skins/default/xui/en/floater_save_pref_preset.xmlindra/newview/skins/default/xui/en/floater_scopes.xmlindra/newview/skins/default/xui/en/menu_scopes_pane.xmlindra/newview/skins/default/xui/en/menu_viewer.xmlindra/newview/skins/default/xui/en/notifications.xmlindra/newview/skins/default/xui/en/panel_lightbox_lens.xmlindra/newview/skins/default/xui/en/panel_lightbox_look.xmlindra/newview/skins/default/xui/en/panel_lightbox_scene.xmlindra/newview/skins/default/xui/en/panel_lightbox_sky.xmlindra/newview/tests/alcolorwheelmodel_test.cppindra/newview/tests/alcurvemodel_test.cppindra/newview/tests/aldaycyclelandmarks_test.cppindra/newview/tests/algradehistory_test.cppindra/newview/tests/alscopedata_test.cppindra/newview/tests/alwhitebalancesolver_test.cppindra/newview/tests/lutcube_test.cppindra/newview/viewer_manifest.py
Sixteen changes answering the review on the upstream PR. The two with teeth: - saveSnapshot now forwards its layer type to rawSnapshot. It sat one positional slot past the no_post fix and was stranded the same way, so a depth snapshot requested through this path came back as colour. - lutcube's parseLine initialises its triple and returns as soon as splitTripel rejects a line, instead of pushing indeterminate floats through the quantiser into a cube that parse() was about to discard anyway. The allocation comment now does its arithmetic at 16 bits per channel, and the size cap drops to 128 accordingly -- still room for every size an authoring tool emits. The rest, smaller: the scope resize drops the stale readback (its histograms stay -- shares are still true after a resize); the curve editor and colour wheel gain belt-and-braces enabled guards on paths parent dispatch already filters; blendToward tolerates empty-but-equal grids; the Looks watcher grows the same isNull guard as its sibling branches and builds its whitelist once; the three manual re-entry flags become ScopedTrue with scopes that close before their trailing refreshes; LutCube::parse is documented one-shot; the double include guard and a stale test constant go; the grading master checkbox is named for what it controls; and LIGHTBOX.md gains the lutcube row, loses its references to checker scripts that never shipped, and derives the top bar's 412 so it cannot be misread against a section row's 361. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enabling Follow Mouse left the whole image in focus. cursorIntersect overwrites gDebugRaycastIntersection with mouse_world_end, the point 512m down the mouse ray, and that global is what the Follow Mouse branch of renderDoF reads as its focus point. A subject distance of 512m gives a blur constant near zero, so nothing was ever blurred. 7f87fcc guarded that write with the follow pointer setting when the feature was added, alongside the existing flycam guard. An upstream merge later resolved the hunk in favour of Linden's side and dropped it. Nothing failed to compile, because 698ae95 removed the LLPipeline member the guard referenced. Restore the guard. The other half is that nothing kept the intersection fresh. The only per frame cursor raycast is gated on the raycast debug mask, so the feature was relying on whichever tool happened to run a hover pick, which stops entirely while the pointer is over the UI. Give it its own raycast in updateUI, gated on the feature being on and unlocked and on depth of field being enabled. cursorIntersect only writes the global on a hit, so a miss holds the last focus point rather than throwing focus out to the far end of the ray. Rigged geometry is picked because mesh avatars are the usual subject. Also fix the mouselook branch, which passed its result as the normal out parameter and left intersection null, so it focused on a surface normal instead of the hit point. bc93177 widened the cursorIntersect signature upstream and shifted the argument by two positions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 0b24576)
1 participant
Description
Expansion of Lightbox to include all of the new shader features, including the colorgrading suite. This also includes new XUI widgets and their documentation. (Interactable Graph, Split View Selector, Colorpucks, and an optional sub-element for Accordions). Some debugs have been left out or made into discreet combo boxes to avoid undesired behavior or to prevent shader and/or realloc thrashing.
Lightbox's floater has been dramatically altered, instead of flat tabs, it now consists of a tab and accordion layout. This allows for tighter grouping and hiding of options that a user does not necessarily need at the moment. Documentation on the workflow to add and hook up additional categories has been provided as part of this.
Beyond XUI alterations there are 7 core features to go along with Lightbox V2: Looks System, Colorgrading XUI Widgets, Lightbox Undo/Redo, White Balance Picker, Scopes, Method Bypass, and Daycycle Scrubbing
Looks System
With V2, Lightbox now has a preset system called the Looks System. Color Grading and Lens Tab's individual settings can be saved as individual presets to recall at any time. Additionally, the floater checks for any altered settings and notifies users that a preset is 'dirty' via a * on the preset's combo box entry. There is a revert button that can clear any dirty changes of the preset. Scene and Sky are too situational to store as Looks presets and are skipped. Three presets ship with this: Golden Hour, Neutral, and Soft Film.
Colorgrading Widgets
As part of the expansion of Lightbox, two critical XUI widgets have been created: Interactable Graph and Colorpuck. The Graph is used to render the Split Tone and the Tone Curve. Graph Widgets can have any number of defined controls that are interacted via mouse drags. This is likely the most universal of the added widgets due to its flexibility. The more specific widget created is the Color Puck. It is a color ring with an internal touch surface designed to have multiple helper elements, such as sliders and mini text input boxes. These are modeled after professional photo and image editing software to feel familiar with those users.
Lightbox Undo/Redo
Due to the mouse-drag heavy XUI elements that Lightbox houses, Lightbox now includes a dedicated undo-redo system that has a 0.5 second snapshot window when dragging controls. While the floater is focused, the OS's/Viewer's shortcut keys for Undo/Redo are intercepted. There are also state-aware buttons that accomplish the same behavior.
White Balance Picker
Lightbox>Look>Basic: Pick Neutral. This will start an eyedropper state that uses the selected pixel to attempt to auto-adjust color temperature and tint to bring it to true neutral. As a precaution, it will reject any sample that does not appear to be grey with a system toast notification describing the failure.
Scopes
To go along with the more granular colorgrading Alchemy provides, a multi-graph scopes floater has also been created, this has Histograms (Combined, Luma, R, G, B), Waveform (Luma, RGB), RGB Parade, and a Vectorscope aligned to the representation of the Colorpucks. This system uses a reduced sized representation of the pre-UI render surface to sample and probe, done primarily to reduce sampling noise. All scope modes are accessible through right click context menus. A toggle for viewing the scopes logarithmic scale is also provided.
Method Bypass and Split Reference
There are three ways to compare Alchemy's colorgrading state: A temporary Hold-To-Skip control binding (unbound by default), Method Bypass (the checkbox on the colorgrading accordion catagories), and the Freeze and split reference under the Colorgrading master accordion. These options should allow for a comparison method that any user should would be comfortable with.
Daycycle Scrubbing
For more control over lighting conditions, the Sky tab holds a Daycycle scrubber, allowing a user to freeze time and scrub through any percentage of the currently used daycycle. There are 4 preset buttons as well. These presets compare sun/moon position, rather than setting a percentage along the scrub. This system uses the existing piping for local environments and clears normally with Use Shared Environment. This also means RLVa restrictions should prevent use as it expects.
Additional Notes