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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Inspector] Prevent crashing when a mesh's name is of the wrong type. #13893

Merged
merged 5 commits into from May 25, 2023

Conversation

carolhmj
Copy link
Contributor

A user can mistakenly set a mesh's name with the wrong type (not string), like this example: https://playground.babylonjs.com/#XT7L5S, where trying to open the Inspector makes it crash. This PR just adds some extra type checking to avoid it crashing, and also adds a quick check to warn the user when adding to a scene.

@bjsplat
Copy link
Collaborator

bjsplat commented May 24, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented May 24, 2023

…Grids/parentPropertyGridComponent.tsx

Co-authored-by: Raanan Weber <raananw+github@gmail.com>
Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

I am not sure we should do this. This is adding extra code and why doing it here and not for all the other APIs we have ?

I guess the OP of the forum thread should use typescript to not use the API incorrectly ?

There might as well be smthg I did not get.

@Popov72
Copy link
Contributor

Popov72 commented May 25, 2023

I am not sure we should do this. This is adding extra code and why doing it here and not for all the other APIs we have ?

This is not the first time this problem has been reported to us and it's not always easy to find that it comes from a bad type in the mesh names, so dealing with the problem on our side would be nice, but as you say, why do it only for this one and not for everything else that could go wrong... It's a bug on the user side, after all, not in Babylon.js. So, after a second though, I would also vote not to do it.

@RaananW
Copy link
Member

RaananW commented May 25, 2023

I don't see a warning in the scene as something critical, but I do think the solution for the inspector is important.

This happens from time to time. We can stay defensive, at least in our tools. This is obviously not "our issue", but warning the user they did something wrong is not a bad thing :-)

If you feel it's not the right place, maybe remove the scene changes. The playground changes should stay, as this breaks the inspector.

@sebavan
Copy link
Member

sebavan commented May 25, 2023

Love the solution of doing the inspector only :-)

@carolhmj carolhmj requested a review from sebavan May 25, 2023 13:38
@sebavan sebavan merged commit d603474 into BabylonJS:master May 25, 2023
8 checks passed
@carolhmj carolhmj deleted the preventInspectorCrash branch July 19, 2023 13:33
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