Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand Invalid RID Error Messages #64234

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Conversation

TokisanGames
Copy link
Contributor

@TokisanGames TokisanGames commented Aug 10, 2022

Differentiates error messages between null, invalid RIDs and those that are not found when calling the free() functions in Visual, Physics, and Navigation Servers.

Addresses @lawnjelly 's comments:
#55764 (comment)

Perhaps if the message "Invalid RID" is now being used for a NULL RID with the first check, these messages at the end could be something like "RID not found."? 馃

That might be handy to differentiate the two cases (although I doubt this later case will happen often).

and
#64199 (comment)

The other problem as I mentioned in the PR, is that this error message does not distinguish between a NULL RID and a RID not found.

@akien-mga

@lawnjelly
Copy link
Member

lawnjelly commented Aug 11, 2022

What do these error messages look like out of interest?

I'm just wondering if we go to the length of writing the server type in the message, maybe it makes sense to disambiguate e.g. BulletPhysicsServer from GodotPhysicsServer, etc.

I suspect the error message currently does show the source file in the Godot IDE, but imo if we are going to show this info in the error message itself, it is kind of strange to not show as much as possible. So I would be tempted to either go with "Invalid RID" and "RID not found" and rely on the file / function info, or write the whole caboodle in the message. 馃

@TokisanGames
Copy link
Contributor Author

TokisanGames commented Aug 11, 2022

It already shows bullet, sw, 2D, etc in the code line.

ERROR: VisualServer attempted to free an invalid RID.
   at: (servers/visual/visual_server_raster.cpp:69)

Remember users, who only know about physics server and visual server, and not the individual types will also see these messages. Invalid RID is quite cryptic, and a lot less information than VisualServer attempted to free an invalid RID, which is what happened.

I can change it to whatever is directed.

@lawnjelly
Copy link
Member

OK, agree.

I'm also wondering whether "attempted to free a NULL RID" might be more informative. 馃

Although the c++ code uses "valid", it's kind of vague term, it could mean corrupt or outdated or anything. Whereas this is specifically for a RID that has not been allocated, or has been set to NULL.

@TokisanGames
Copy link
Contributor Author

OK, I'll change it to null.

@TokisanGames
Copy link
Contributor Author

The messages have been updated with NULL RID.

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Looks great! Sorry for delay was away for weekend.

@clayjohn clayjohn merged commit 24a9812 into godotengine:3.x Aug 15, 2022
@clayjohn
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants