build!: add amalgamation system #44
No reviewers
Labels
No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
mupen64/ugui!44
Loading…
Reference in a new issue
No description provided.
Delete branch "amalgamation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Splits BreitbandGraphics, ugui, and ugui-ext into multiple scripts which are unified into
breitbandgraphics-amalgamatedandugui-amalgamatedusing a new amalgamation system.Breaking Changes
The built library filenames have changed.
mupen-lua-ugui.lua->ugui-amalgamated.luamupen-lua-ugui-ext.lua->ugui-amalgamated.luabreitbandgraphics.lua->breitbandgraphics-amalgamated.luamupen-lua-ugui-ext.luadoesn't exist anymore - it's now integrated inugui.Consequently, all other members of
ugui_ext(e.g.ugui_ext.internal.rectangle_to_key) have now been moved intougui. If you are overriding or otherwise hacking around with these, make sure to adjust that code as well.the global
mathshims were removed due to them being non-standard and now part of Mupen64.If you are seeing errors like
attempt to call a nil value (field 'pow'), you should update Mupen64.Pull request overview
This PR introduces an amalgamation build system that splits BreitbandGraphics and ugui into multiple source files which are combined during build. The system uses a Python script to merge files with proper headers.
Changes:
src/directorybuild.pyto amalgamate source files intobuild/directorymupen-lua-ugui-ext.lua- now returned as second value from ugui-amalgamated.luaReviewed changes
Copilot reviewed 30 out of 50 changed files in this pull request and generated 3 comments.
Show a summary per file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -0,0 +39,4 @@return {primary = selected_index,meta = {{ signal_change = data.signal_change },The meta table is incorrectly wrapped in an extra table. It should be
signal_change = data.signal_changedirectly, not{ signal_change = data.signal_change }. This will cause the meta field to be a table containing a table instead of the expected Meta structure.@ -0,0 +103,4 @@data.signal_change = ugui.internal.process_signal_changes(data.signal_change, control.value ~= data.value)return {value = data.value,The numberbox logic returns an object with field
valueinstead ofprimary. This is inconsistent with all other controls which return{ primary = ..., meta = ... }. The calling code expects result.primary, which will be nil here.@ -0,0 +55,4 @@is_checked = selected_index == i,})if not previous == new thenThe comparison logic is inverted. The condition
not previous == newshould beprevious ~= new. The current logic treats the boolean result ofprevious == newas a table/value to negate, which will likely always evaluate to false (since any value negated is false), causing the selected_index to always be updated.Pull request overview
Copilot reviewed 31 out of 51 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
src/ugui/ugui-ext.lua:59
ugui_ext.internal.cached_draw(andugui_ext.free) are only defined whend2d.draw_to_imageexists. Ifd2d.draw_to_imageis absent butd2d.create_render_targetis present, the fallback branch (if not d2d.create_render_target and not d2d.draw_to_image) won't run either, leavingcached_draw/freenil and causing runtime errors whenapply_nineslicecalls them. Either reintroduce thecreate_render_targetimplementation, or make the fallback trigger wheneverdraw_to_imageis unavailable socached_draw/freeare always defined.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -0,0 +1,443 @@--BreitbandGraphics.draw_imagecallscolor_source_to_float_color(color)but that function isn't in scope here (the implementation lives atBreitbandGraphics.internal.color_source_to_float_color). Also theelsebranch assignsBreitbandGraphics.colors.white(a Color) tofloat_colorand later usesfloat_color.a, which will be nil. UseBreitbandGraphics.internal.color_source_to_float_colorfor both branches (or otherwise ensurefloat_coloris a proper{r,g,b,a}float table).@ -0,0 +30,4 @@r = tonumber(hex:sub(2, 3), 16),g = tonumber(hex:sub(4, 5), 16),b = tonumber(hex:sub(6, 7), 16),}hex_to_colorcurrently usesreturnon its own line followed by a table on the next line. In Lua this returnsnil(the table is treated as a separate statement), so both branches will returnnilinstead of a color table. Put the table literal on the same line asreturn(or wrap in parentheses) so the function actually returns the constructed color.@ -0,0 +42,4 @@r = value,g = value,b = value,}repeated_to_colorhas the same Lua issue ashex_to_color:returnis on its own line, so the function returnsnilinstead of the table. Keep the returned table on the same line asreturnso callers receive a valid{r,g,b}color.@ -0,0 +36,4 @@r = (source >> 24) & 0xFF,g = (source >> 16) & 0xFF,b = (source >> 8) & 0xFF,a = source & 0xFF,color_source_to_float_color's RawColor branch returns channel values in the 0–255 integer range, but the function claims to return a FloatColor (0.0–1.0) and its callers pass the result directly to APIs liked2d.create_brushwhich expect floats. Convert these extracted bytes to floats (divide by 255.0) to avoid incorrect colors/alpha.@ -0,0 +39,4 @@return {primary = selected_index,meta = {{ signal_change = data.signal_change },carrousel_button's registrylogicreturnsmeta = { { signal_change = ... } }(an array containing a table) instead of a plainMetatable. This makesresult.meta.signal_changenil for callers. Returnmeta = { signal_change = data.signal_change }to match theControlReturnValue/Metacontract used by other controls.@ -0,0 +10,4 @@validate = function(control)ugui.internal.assert(type(control.items) == 'table', 'expected items to be table')ugui.internal.assert(type(control.selected_index) == 'number', 'expected selected_index to be number')end,ugui.registry.combobox.validaterequiresselected_indexto be a number, but the public type (ComboBox.selected_index) is documented as optional/nil-able and the styler handlesnilas "no selection". Allownilhere (or update the type/docs) so callers can create an unselected combobox without validation errors.@ -0,0 +150,4 @@endreturn result, result.metaendugui.menutreats the return value ofugui.control(control, 'menu')as if it were aMenuResult(result.item,result.dismissed), butugui.controlreturns aControlReturnValuewrapper{ primary = MenuResult, meta = ... }. This breaks both submenu propagation and the public return type (callers will receive the wrapper table). Mirror the other controls: work withresult.primaryand returnresult.primary, result.meta.@ -0,0 +103,4 @@data.signal_change = ugui.internal.process_signal_changes(data.signal_change, control.value ~= data.value)return {value = data.value,ugui.registry.numberbox.logicreturns{ value = data.value, meta = ... }, butugui.controland all other registry entries use{ primary = ..., meta = ... }. Returningvaluemeans callers will seeresult.primary == niland breaks the control contract. Rename the field toprimary(and keepmetaas-is) sougui.controlwrappers behave consistently.@ -0,0 +196,4 @@endreturn math.floor(data.value), data.metaendugui.numberboxreturnsdata.meta, but the registry logic never stores ametafield intocontrol_data(onlysignal_change). This will returnnilfor Meta even when the control changes. Capture theControlReturnValuefromugui.control(control, 'numberbox')and return its.meta(updatingsignal_changeif you mutatedata.valueafterward).@ -0,0 +55,4 @@is_checked = selected_index == i,})if not previous == new thenif not previous == new thenis parsed as(not previous) == new, so it won't reliably detect changes in selection state. Useif previous ~= new(orif new and not previous) to correctly updateselected_indexwhen a different tab becomes checked.@ -0,0 +14,4 @@---@param control ToggleButton---@return ControlReturnValuelogic = function(control, data)data.is_checked = control.is_checkedugui.registry.toggle_button.validateassertscontrol.is_checkedis a boolean, but the public type (ToggleButton.is_checked) is optional and documented as defaulting to unchecked when nil. Acceptnilhere (treat as false) to match the documented API and avoid forcing callers to always passis_checked.@ -0,0 +513,4 @@endcolor = resultendtable.insert(segments, { type = 'icon', value = icon_name, color = color ~= '' and color or nil })parse_rich_textusescolor:find('.')to detect whether the color is a param-path, but in Lua patterns.matches any character, so this condition is true for any non-empty color (including hex colors like#FFFF00). That will incorrectly treat hex colors as paths and likely resolve to nil. Use an escaped dot pattern ('%.')and/or explicitly detect hex strings (e.g.color:sub(1,1) == '#').@ -0,0 +579,4 @@-- Determine the hovered control if we haven't alreadyif ugui.internal.hovered_control == nil thenif is_point_inside_rectangle(ugui.internal.environment.mouse_position, control.rectangle) thenugui.internal.hovered_control = control.uiddo_input_processinguses a localis_point_inside_rectanglethat ignoresugui.internal.is_point_inside_control(enabled state + window bounds). As a result, controls can be hovered/clicked even when the mouse is outside the window (negative coords), which contradicts the intended offscreen hittest behavior and makesis_point_inside_controlunused for core input routing. Consider usingugui.internal.is_point_inside_control(or replicating its checks) when determining hovered/clicked controls.@ -0,0 +717,4 @@height = rectangle.height,}, -7)ugui.standard_styler.draw_icon(icon_rect, ugui.standard_styler.params.menu_item.height, nil, 'arrow_right')enddraw_menu_itempassesugui.standard_styler.params.menu_item.height(a number) as thecolorargument todraw_iconfor both the checkmark and submenu arrow. This will be treated as a RawColor integer and produce incorrect rendering. Pass an actual color (e.g.ugui.standard_styler.params.menu_item.text[visual_state]or another dedicated icon color) instead of the height.