fix: No longer check IsPrimitiveType when nodePresenter.value is null to prevent crash when selecting multiple entities in GameStudio

[Acissathar]

PR Details

Attempting to select multiple entities may crash GameStudio due to an ArgumentNullException during the IsPrimitiveType check, as type is sometimes null. Selecting the entities individually does not appear to cause the issue.

I can recreate this pretty consistently with a New Game template specifically when multi-selecting the materials, as the Tessellation node property has a value of null. I am otherwise unable to recreate it with any other object/asset/entity types.

Given its specifically (for me at least) this Tessellation node property, I'm not entirely sure if this is more of a band-aid that is hiding something that should be dived into (like why is Tessellation node property null at this point) or not.

Running that guard locally seems to solve the problem though, and it still shows the property grid as expected.

Related Issue

#2903

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[Acissathar]

As a follow up, I was finally able to recreate it with other entity types, such as Skeletons and Models, so I don't think it's specific to just Tessellation which makes me feel a little better about the more blanket return false.

[Eideren]

Given that this method does not specify that it accepts null, it should be fixed at the callsite of the method instead, and handled appropriately there.
The log specifies that it throws at

stride/sources/presentation/Stride.Core.Presentation.Quantum/ViewModels/NodeViewModel.cs

Line 355 in ce72d51

else if (nodePresenter.Factory.IsPrimitiveType(nodePresenter.Value?.GetType()))

[Eideren]

Looks good to me but not really my area - pinging @Kryptos-FR if he has time take a quick look ?

[Kryptos-FR]

Good catch. I have a different fix in my xplat-editor branch (see 70b50bb):

Suggested change

	else if (nodePresenter.Value != null && nodePresenter.Factory.IsPrimitiveType(nodePresenter.Value.GetType()))
	else if (nodePresenter.Factory.IsPrimitiveType(nodePresenter.Value?.GetType() ?? nodePresenter.Type))

Can you check if that also solves the issue (and don't introduce a different regression)? I'd prefer to have that version if it also works as it seems more correct to me.

To be honest, I think both fixes are equivalent. I can't think of a case where that would make a difference. But who knows?

[Acissathar]

@Kryptos-FR I ran it through the same tests I did of the original and this change handles them. I also ran it through my experimental yes to all prompt, which previously would also duplicate the issue, and this change handles it as well.

Granted for context my test is largely just continually ctrl + click selecting assets in the asset view along with tabbing to shift focus, as that was the only way I was able to replicate it (outside my experimental feature which did it every time).

Not sure if it was recreated other ways, but that's also how the original issue was written up, so I think this handles the issue.

[Eideren]

Thanks !