Load icon textures from a memory buffer - #7
Open
LamaQuest wants to merge 1 commit into
Open
Conversation
NodeCore::LoadTextureFromBase64 decoded the built-in icons, wrote them to "TemporaryIcon.png" in the process working directory, handed that path to the texture loader and deleted the file again -- eight times, as soon as SetTextureLoader is given a non-null loader. That round trip through the disk is unnecessary work in every case, and it is impossible in some: the path is a bare relative name, so it lands wherever the process happens to be running, which may be read-only, may be shared between concurrently running instances, or may simply be a directory the host is not allowed to write to. Nothing in the library needs a path -- LoadTextureFromBase64 is the only caller of the loader, and it always has the bytes in hand. The loader now receives the decoded bytes directly, as a pointer and a size. Every image decoder in common use has a load-from-memory entry point, so a client keeps the same amount of code; a client that really wants a file can still write one itself, which is the choice it should have been making all along. This changes the signature of SetTextureLoader, which is public API. Keeping a path-taking overload alongside it was considered and rejected: the library has no path to give such an overload, so it could only be served by writing the temporary file again -- that is, by keeping the defect this patch removes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi,
First, thank you for VisualNodeSystem. I went looking for a node-graph library
that stays out of the way — no window, no event loop, no device ownership — and
yours was the only one I found that actually delivers on that. It made a design
possible that I had almost given up on.
For context: I'm building a visual scripting runtime for Skyrim Special Edition,
where the node editor opens over the running game from inside an in-process
plugin. That is how I ran into the issue below — in that setting the process
working directory is the game's install folder, which is often read-only.
The problem
NodeCore::LoadTextureFromBase64decoded the built-in icons, wrote them to"TemporaryIcon.png"in the process working directory, handed that path to thetexture loader and deleted the file again — eight times, as soon as
SetTextureLoaderis given a non-null loader.That round trip through the disk is unnecessary work in every case, and it is
impossible in some: the path is a bare relative name, so it lands wherever the
process happens to be running, which may be read-only, may be shared between
concurrently running instances, or may simply be a directory the host is not
allowed to write to. Nothing in the library needs a path —
LoadTextureFromBase64is the only caller of the loader, and it always has thebytes in hand.
The change
The loader now receives the decoded bytes directly, as a pointer and a size.
Every image decoder in common use has a load-from-memory entry point, so a
client keeps the same amount of code; a client that really wants a file can
still write one itself, which is the choice it should have been making all
along.
On the API break
This changes the signature of
SetTextureLoader, which is public API. Keeping apath-taking overload alongside it was considered and rejected: the library has
no path to give such an overload, so it could only be served by writing the
temporary file again — that is, by keeping the defect this patch removes.
Testing
Tested against the
testsbranch withMasterBranchCodepointed at thiscommit: 306 / 306 passing. The suite compiles unchanged against the new
signature — no test used the path-based form.
Migration for existing hosts
One line:
stbi_load(path)becomesstbi_load_from_memory(data, size). Everycommon decoder has a from-memory entry point, and the signature change surfaces
as a compile error, never as a silent runtime failure.
I'm aware this breaks public API, so it is entirely your call. If you'd rather
keep the current signature, or shape the change differently, just say so and
I'll adapt — or drop it, no hard feelings either way. Please don't feel any
pressure on timing; nothing on my side is waiting on this.
Thanks again for the library.