Skip to content

Initialize to empty path if .strings.bank not loaded - #429

Open
ambiguousname wants to merge 2 commits into
utopia-rise:masterfrom
ambiguousname:error_on_strings_not_loaded
Open

Initialize to empty path if .strings.bank not loaded#429
ambiguousname wants to merge 2 commits into
utopia-rise:masterfrom
ambiguousname:error_on_strings_not_loaded

Conversation

@ambiguousname

Copy link
Copy Markdown

As a follow up to #428, an error/warning message would be useful here to help people in situations like mine diagnose the problem more easily.

Since not everyone might want to load in strings, happy to make this a warning instead; also happy to move this message to more bus/event specific loading code. Let me know what would work best.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds diagnostic logging to help users understand why FMOD objects fetched by path (e.g., buses/events) may fail at runtime when the corresponding strings bank isn’t loaded (follow-up to #428).

Changes:

  • Replaces the silent getPath(...) check in FMODCLASSWITHPATH with ERROR_CHECK_WITH_REASON(...) to emit a helpful message when path resolution fails.

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

Comment thread src/helpers/common.h Outdated
Comment on lines 69 to 72
char path[MAX_PATH_SIZE]; \
ERROR_CHECK(wrapped->getPath(path, MAX_PATH_SIZE, nullptr)); \
ERROR_CHECK_WITH_REASON(wrapped->getPath(path, MAX_PATH_SIZE, nullptr), "Could not find path. It is likely that .strings.bank has not been loaded."); \
ERROR_CHECK(wrapped->getID(&ref->_guid)); \
ref->_path = String(path); \

Copilot AI Mar 28, 2026

Copy link

Choose a reason for hiding this comment

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

wrapped->getPath(...) can fail (e.g., when the strings bank isn’t loaded), but the return value from ERROR_CHECK_WITH_REASON(...) is currently ignored. If it fails, path remains uninitialized and ref->_path = String(path) can read garbage / run past the buffer (undefined behavior) while also continuing to populate _guid. Handle the failure explicitly (e.g., initialize path to an empty string and/or early-return an empty Ref or set _path to "" when getPath fails).

Copilot uses AI. Check for mistakes.
@ambiguousname

Copy link
Copy Markdown
Author

I noticed the PR checks timed out; I currently have FMODPASS and FMODUSER as secret variables in my fork if that helps.

I'm also not sure if you'd like me to address Copilot's review: the feedback seems to be outside of the scope of "add an error message to a pre-existing error". I'd be happy to expand scope to include more checks for the result of getPath, if that's a preferable solution.

Let me know if there's any other concerns I can address.

@CedNaru

CedNaru commented Apr 27, 2026

Copy link
Copy Markdown
Member

Hello.
So from what i undestand, we would still get an error if no string bank is used, which doesn't go well if one with to use pure guid. So probably, trying to get the path itself shoudln't trigger an error and silently pass set the path to a null value.
Then if one wish to use that path in anyway, the getter for the path shoud in this case raise the error and print that message.
What do you think?

@ambiguousname

Copy link
Copy Markdown
Author

I think that makes sense to me. I will update this PR when I have some time to include those extra checks.

@ambiguousname

Copy link
Copy Markdown
Author

Finally had some time to test this, I improved the error checker, initialized the path with empty values (in case loading fails), and added a message which prints the guid of the event that fails to load.

@ambiguousname

Copy link
Copy Markdown
Author

Ah, and I forgot to address the feedback about moving the errors to the getter, will do so shortly

@ambiguousname

ambiguousname commented Jul 26, 2026

Copy link
Copy Markdown
Author

So, I looked into implementing the above, and I don't think inserting errors into the getter works because of the caching system. When a bank is loaded, strings_to_guid is updated with the getter get_path, leading to an error on bank load regardless if the user intends to use strings.bank or not.

The cache emits warnings for the editor/debug builds when it cannot find a path from strings_to_guid, so maybe the fix here is to just initialize to an empty path on a failed load and let the existing cache warnings at least signpost where something has gone wrong.

Related to this, I found a bug in _create_event_instance where it does not check the validity of the event description before attempting to get its inner wrapper (i.e., when .strings.bank is unloaded). I've updated this PR to include a fix for that.

Edit: Just found another similar bug in FMODServer.play_one_shot

@ambiguousname
ambiguousname force-pushed the error_on_strings_not_loaded branch from 6463b7c to bf4bf17 Compare July 26, 2026 16:28
@ambiguousname
ambiguousname force-pushed the error_on_strings_not_loaded branch from bf4bf17 to 31b595d Compare July 26, 2026 16:39
@ambiguousname ambiguousname changed the title Add error message on path not found Initialize to empty path if .strings.bank not loaded Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants