Skip to content

Conversation

GabrielBigardi
Copy link

@GabrielBigardi GabrielBigardi commented Sep 8, 2025

Purpose of this PR

Using .ToLower() to compare strings is very inefficient as it allocates a new string. Using string.Equals with StringComparison.OrdinalIgnoreCase is not only a lot faster but also handles some problems that may happen on different cultures, as shown in this video from "Bald. Bearded. Builder.": https://www.youtube.com/watch?v=DNzAoLwXLzc

Changelog

  • Fixed: Inefficient string comparison on CommandLineHandler.

Using .ToLower() to compare strings is very inefficient as it allocates a new string. Using string.Equals with StringComparison.OrdinalIgnoreCase is not only a lot faster but also handles some problems that may happen on different cultures, as shown in this video from "Bald. Bearded. Builder.":
https://www.youtube.com/watch?v=DNzAoLwXLzc
@GabrielBigardi GabrielBigardi requested a review from a team as a code owner September 8, 2025 19:40
@unity-cla-assistant
Copy link

unity-cla-assistant commented Sep 8, 2025

CLA assistant check
All committers have signed the CLA.

@sentinel-u3d sentinel-u3d bot requested a review from EmandM September 9, 2025 17:56
Copy link
Collaborator

@EmandM EmandM left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for this submission.

Out of interest, which issue did you see that needs this fix?

I ask as this script is inside our test project in a path that is unlikely to be called often. Is there a reason this optimization is needed here?

@GabrielBigardi
Copy link
Author

GabrielBigardi commented Sep 9, 2025

Hi! Thanks for this submission.

Out of interest, which issue did you see that needs this fix?

I ask as this script is inside our test project in a path that is unlikely to be called often. Is there a reason this optimization is needed here?

Hello!

Apart from being a way faster way of comparing these strings (which may helps a lot on older/weaker devices), it also fix a problem that may be caused by different cultures in C#. Using .ToLower() for comparison won't work for Turkish and a few other countries.
A simple test can be made by adding the following code at the top of the class:
Thread.CurrentThread.CurrentCulture = new CurrentCulture("tr-TR");

If the culture is set to turkish, sceneLoaded.name is "IndieGame" and m_SceneToLoad is "indiegame" for example this check will fail.

@EmandM
Copy link
Collaborator

EmandM commented Sep 9, 2025

Thanks for the reproduction!

Just for our knowledge, how are you using the NGO test project in your environment?

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.

3 participants