Add GUI to Memory Panels

[rubyswolf]

Added a GUI to the memory panel which allows you to view and edit the stored data as a comma separated list of key:value entries like "0:1,1:2"

[JuneCarya]

I have some questions:

1. Can you disclose whether you used an LLM to assist you in writing the code for this PR?
2. Cold you explain the mechanisms that this PR introduces to read and save key-value pairs with references to the code segments that correspond to (i) reading and displaying values, (ii) interpreting the entire input and checking for its validity, and (iii) how it saves the data?
3. Did you run extensive tests on your changes, confirming that they don't intoduce braking changes for earlier creations, and check that everything behaves as expected?

[JuneCarya]

I've added some comments to some code segments.
This isn't a finalised review and I will leave up the decision to merge to one of the maintainers

[0x57e11a]

feel free to ignore the nitpicks, they're mostly just style

[rubyswolf]

I have some questions:

1. Can you disclose whether you used an LLM to assist you in writing the code for this PR?
2. Cold you explain the mechanisms that this PR introduces to read and save key-value pairs with references to the code segments that correspond to (i) reading and displaying values, (ii) interpreting the entire input and checking for its validity, and (iii) how it saves the data?
3. Did you run extensive tests on your changes, confirming that they don't intoduce braking changes for earlier creations, and check that everything behaves as expected?

Yes I used jules (gemini) and gpt to try to accelerate parts of the development. most of the code was fairly simple and I have adjusted, reviewed and tested.
It just reads the data by accessing self.data and writes through self.storage:save(saveData) like the rest of the code does. The values are form
the part that reads and displays the data is just this:

	local parts = {}
	for k, v in pairs(self.data) do
		parts[#parts + 1] = k .. ':' .. v
	end

	self.mem_gui_input = wrapInput(table.concat(parts, ","))

the validation part which was generated is:

	for entry in input:gmatch("([^,]+)") do
		-- trim surrounding whitespace
		entry = entry:match("^%s*(.-)%s*$") or ""
		if entry ~= "" then
			-- require exactly one ':' in the entry (allow empty lhs/rhs)
			local addrStr, valStr = entry:match("^([^:]*):([^:]*)$")
			if addrStr then
				-- parse address to integer, make positive
				local addr = tonumber(addrStr) or 0
				if addr < 0 then addr = -addr end
				addr = math.floor(addr)

				-- parse value (prefer numeric if looks like a number)
				local numVal = tonumber(valStr)
				local savedVal
				if numVal and numVal == numVal then
					savedVal = tostring(numVal)
				else
					-- if empty, default to "0"
					if valStr == "" then
						savedVal = "0"
					else
						savedVal = valStr
					end
				end

				-- assign into data table
				data[addr] = savedVal
			end
		end
	end

which upon review I can see has a little oversight in the fact that the LHS and RHS can be blank and default to zero when I believe it should just ignore such an entry.
I have tested the features ingame to see that they work as expected when used correctly. I also tested invalid entries like "1:2:3", "1" and "" to see that they are ignored and don't break anything. I have only tested at 1080p so I haven't validated that the wrapping behaviour works at all resolutions. I understand the formatting and readability concerns, my main priority was compactness as you might need to enter a lot of data and I wanted it to be as easy as possible to scroll down to a very large address without it taking long. If readability is more important then this can be changed. maybe a format setting could be introduced. I expect the main usecase of this feature to be people generating data with code and then pasting it in so I personally don't see the readability as that important.
let me know if there are any more concerns

[rubyswolf]

oh alright I'll format it this way then and then expand it vertically, it looks ok

[0x57e11a]

this looks a lot better!

[rubyswolf]

All requested changed were made and more

[rubyswolf]

also sidenote, the code for the counter GUI also creates it every time on tinker and I just copied that behaviour. If it's better to create only once then that should be applied to the counter too.

[rubyswolf]

same for the math block

[rubyswolf]

I know you're supposed to make each feature into its own pull request but it's so minor that I hope you'll let it slide. up to you guys how you handle it.

[rubyswolf]

it might actually be intentional to avoid loading a whole GUI instance for every block every time even when it's not going to be used. sounds like it would be a memory usage concern.

[JuneCarya]

also sidenote, the code for the counter GUI also creates it every time on tinker and I just copied that behaviour. If it's better to create only once then that should be applied to the counter too.

Yeah I've just seen that now too and it's valid to just use the same approach there in this case. I still would've probably approached it differently, but that's okay.

it might actually be intentional to avoid loading a whole GUI instance for every block every time even when it's not going to be used. sounds like it would be a memory usage concern.

Fair point. I don't know about the intentions of the original developers who worked on this. If it comes to having a single GuiInterface instance that would be used across all shape instances, a script-local variable could be used, which contains a reference to the GuiInterface instance:

local memoryGui
...

function MemoryPanel:client_onCreate()
    ...
    if not memoryGui then
        memoryGui = sm.gui.createGuiFromLayout("$CONTENT_DATA/Gui/Layouts/MemoryPanelGui.layout", false, { backgroundAlpha = 0.5 })
        -- Rest of the setup code
    end
    ...
end
...

I suppose you can ignore my earlier comment about moving the creation of the GuiInterface object in client_onCreate and revert that change. It's up to you

[rubyswolf]

I'll do you one better, give me a moment

[rubyswolf]

never mind my idea didn't work, looks like your suggestion really is the best approach

[rubyswolf]

ok so your idea worked on the memory panel UI, but when I tried the exact same approach on the counter and math block, they just both stopped working

[rubyswolf]

I've double checked to make sure I did the replacements just right. the counter block throws no errors but just the button callbacks don't work. and the math block throws errors when you try to select a function.

[rubyswolf]

so I suppose I'll just leave the counter and math blocks alone

[JuneCarya]

Could you also test your changes in multiplayer with other people online?
I suspect some issues with the synchronisation of the server's data with all its clients.

[rubyswolf]

alright I'll try to fix it all and try to get someone to test it tomorrow

[rubyswolf]

the errors occuring were just from data not beng synced

[rubyswolf]

I'm thinking a better way to do it would be to just sync the data once when the GUI is opened as like a request to the server. I'll try that tomorrow and test it with a friend.

[rubyswolf]

Yeah no the cached GUI in a global variable was breaking things again @JuneCarya.
caused more errors in multiplayer because the callbacks detached due to the one global instance, not to mention the random one off error from before. This is not safe and I'm reverting to the way the counter and math blocks handle it.

13:41:45 (2308704/141051) ERROR: LuaManager.cpp:1378 Lua call buffer - callback to unknown instance with script ref. ScriptRef: 3170 ScriptType: 'Generic' Callback: 'client_onTextChangedCallback'
13:42:00 (2309305/143980) ERROR: LuaManager.cpp:1378 Lua call buffer - callback to unknown instance with script ref. ScriptRef: 3170 ScriptType: 'Generic' Callback: 'client_gui_saveWrittenValue'

also according to crackx in the discord:
"also tbh, unless the gui is very complex, imo there isn't really a reason to cache it anyway
the layout files, imagebox images and such are cached internally in the engine already anyway"

[rubyswolf]

tested in multiplayer and working

[rubyswolf]

there is an alternative approach I could take where the client asks the server for the data as the GUI opens but this would introduce latency

[rubyswolf]

currently it keeps all clients synced with the latest state of the memory panel

[0x57e11a]

iio: a little bit of latency sounds like an acceptable tradeoff for avoiding unneeded network spam

[0x57e11a]

other than the decision between less network usage and lower latency, LGTI

thank you for your contributions and your patience with the review process!

[rubyswolf]

Yeah I think it's probably for the best, that should do it

[rubyswolf]

no something still feels off, I don't think setClientData makes sense for this way of doing it because that will send to all clients

[rubyswolf]

I think that's a much neater way of handling it, give it a proper multiplayer test tomorrow

[0x57e11a]

after an mp test and @JuneCarya's review, should be ready to merge!

[rubyswolf]

2025-11-15.13-47-14.mp4

It works exactly as expected!
Small latency for connected clients when opening the GUI but instant for the host.

[JuneCarya]

These will be my final requested changes. After that I'll request one of the maintainers to check this and let them make the final call about implementing this feature.
I'm usually not involved in the decision making on what features are green lit and also don't want to take on that responsibility without some of the original authors getting involved.

[rubyswolf]

Dear maintainers, thank you for your consideration, I believe this to be a very useful feature which many people have been lacking for a while. Until this point the only way to enter large amounts of data into memory panels has been to use macros or blueprint editing which is either slow or difficult. There are a wide variety of applications that memory panels are used for like storing programs for computers and saving video and music data and working with memory panels has often been tedious. This feature adds a simple GUI not unlike the counter which makes viewing and editing the stored data of the memory panel trivial. I hope you see it's potential and will choose to officially add it to the modpack

[JuneCarya]

@TechnologicNick I'll be handing over the decision to you now.
Additional testing by people more familiar with the code base could make sure that everything is fine.

What could be done is rebasing this PR to clean up the commit history should this be merged into main @rubyswolf

[rubyswolf]

@JuneCarya I can't figure how to do it. It just keeps saying that everything is already up to date and I don't see a pull requests settings tab at all like it says there should be online.

[rubyswolf]

Please help

[JuneCarya]

I was a bit busy the last few days and don't check in on github that often.
Regarding rebasing, you can read up about it here https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

I don't use github's desktop application, so I can't help you with that tool. The link above explains rebasing using CLI tools