1

I am trying to make a simple search and reveal for an inventory, wondering if there is a better way to do this? This is attached to the parent Gameobject and works with an InputField to compare the text in the field and the name of the Gameobjects, in order to show and reveal them.

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.UI;
using System.Linq;

public class findFrame : MonoBehaviour
{

    public InputField searchText;

    void Find()
    {
        var found = new List<GameObject>(GameObject.FindGameObjectsWithTag("Button")).Where(g => g.transform.IsChildOf(this.transform));

        foreach (GameObject g in found)
        {
            Debug.Log("hello");

            if (g.transform.name != searchText.text)
            {
                gameObject.SetActive(false);
            }
            else
            {
                gameObject.SetActive(true);
            }
        }
    }


        void Update()
        { 
            Find();
        }
    }
cyo
  • 69
  • 1
  • 14
  • Generally speaking, if your code works and you're looking for someone to review it, you would want to ask on https://codereview.stackexchange.com/. Please read their [On Topic](https://codereview.stackexchange.com/help/on-topic) before posting, if you choose to do so. – Tieson T. Feb 05 '20 at 22:53
  • I do not agree with @TiesonT..... When it comes to game engines code might not "behave" as one might expect. And NO this might not be the best way to compare strings in the Update function which executes constantly.... In Unity you might want to use the objects Tag to compare because unity performs it's "magic" on the Component.CompareTag or GameObject.CompareTag methods during compilation to avoid unnecesary memory allocations.... If you really have to compare two strings the fastest way would be to use string.CompareOrdinal(g.transform.name, searchText.text) != 0 – Jonathan Alfaro Feb 06 '20 at 01:56
  • But in general in Unity the fastest way is to use the objects Tag and use the CompareTag method.... So if I was you... I would set the Tag of the object I want to compare and use maybe something like g.CompareTag(searchText.text) – Jonathan Alfaro Feb 06 '20 at 02:01
  • @JonathanAlfaro If you disagree with the suggestion that this fits better on Code Review, please add an answer explaining what is wrong with the code and what should be done to correct it. – Tieson T. Feb 06 '20 at 02:08
  • @TiesonT. Done... This is a well known issue in Unity. – Jonathan Alfaro Feb 06 '20 at 02:11

2 Answers2

1

Short Answer: NO!

  • In general as already mentioned: Do not do it in Update => every frame! (Btw also not in FixedUpdate which runs less frequent but still adds completely unnecessary calls)

    Rather make your code event driven. In your case you could rather add a listener to InputField.onValueChanged which is only called when the value did actually change which reduces your method cals to the necessary.

  • Then I would rather at the beginning store all objects once in a HashSet (or depending on your needs maybe even a Dictionary in case you want to be able to access a button by its name) which is way more efficient then retrieving the list over and over again each time the method gets called.

  • Also using FindObjectsWithTag is expensive and then filtering out using isChildOf is even more expensive and unnecessary - afaik it basically bubbles up the transform.parent recursively until it either finds the given value or null. Transform implements an IEnumerable returning all child Transforms so that you can simply iterate through all child objects using a foreach. So rather go the other way round and iterate only the children and check if they have the tag using CompareTag.

    This depends on your setup ofcourse since isChildOf also returns true if the given object is a nested (deep) child. In case your buttons actually have specific component attached - like I would e.g. guess in this case a Button - you can overcome this by using GetComponentsInChildren<Button>() and iterate on this instead.

  • Finally for comparing strings there are way more efficient and more importantly more secure ways then simply using ==(see C# difference between == and Equals())

    You could either search for an exact match using string.Eqials

    var isMatch = name.Equals(searchString, COMPARISONTYPE);
    

    Or - what I would prefer - rather find all partial matches by either simply using string.Contains

    var isMatch = name.Contains(searchString);
    

    or in case you need it more customized use string.Compare

    var isMatch = string.Compare(name, searchString, COMPARISONTYPE)
    

    Where for the optional (but recommended) parameter COMPARISONTYPE you can choose one of the available StringComparison values in order to e.g. match case insensitive etc.

So I would use something like

public class findFrame : MonoBehaviour
{ 
    public InputField searchText;

    // A HashSet is basically a list but makes sure every element is unique
    // I would make this rather a Dictionary<string, GameObject> in case
    // you also plan to access a specific button by name
    // otherwise a HashSet does it
    private HashSet<GameObject> childButtons = new HashSet<GameObject>();

    public void Awake()
    {
        //Adds a listener to the input field and invokes a method when the value changes.
        searchText.onValueChanged.AddListener(OnValueChanged);

        // Run through all children GameObjects of this GameObject and check the tag
        foreach(Transform child in transform)
        // or as said if possible/necessary for nested children you could use
        //foreach(var child in GetComponentsInChildren<Button>())
        {
            // check if this child has the correct tag
            if(!child.CompareTag("Button")) continue;

            // If the tag matches add it to the HashSet
            childButtons.Add(child.gameObject);
        }
    }

    // Invoked when the value of the text field changes.
    public void OnValueChanged()
    {
        Debug.Log("Value Changed");

        // Get the search value
        var searchString = searchTexxt.text;

        // Simply run through the HashSet and set the matching object(s) active, the others inactive
        foreach(var button in childButtons)
        {
            // Skip null entries
            if(!button) continue;

            var name = button.name;

            // Here as said you can either check for an exact match
            //var isMatch = name.Equals(searchString, COMPARISONTYPE);
            // Or you could also check for partial matches - I would prefer this
            // (required if you want to see objects also while the user didn't type out the full name yet)
            // either by simply using Contains
            //var isMatch = name.Contains(searchString);
            // or if you need it more customized 
            var isMatch = string.Compare(name, searchString, COMPARISONTYPE) == 0;

            button.SetActive(isMatch);

            if(isMatch) Debug.Log($"Found match for {searchString} in button ${name} ");
        }
    }

    // When creating the buttons on runtime call this to also add them to the HashSet
    public void AddButton(GameObject button)
    {
        if(childButtons.Contains(button)) return;

        childButtons.Add(button);
    }
}
  • If you create the buttons on runtime via a script, well, then make sure you add them to the HashSet everytime you instantiate one.

    // In another script you need the reference to the `findFrame` component
    // Drag it in here via the Inspector
    [SerializeField] findFrame findFrameReference;
    
    // As fallback find it on runtime (in case there is only one in the scene
    private void Awake ()
    {
        if(! findFrameReference) findFrameReference = FindObjectOfType<findFrame>();
    }
    

    Then wherever you create the new buttons

    var button = Instantiate(buttonPrefab, ...);
    findFrameReference.AddButton(button.gameObject);
    
derHugo
  • 83,094
  • 9
  • 75
  • 115
  • Thanks for this! trying to wrap my head around how to add the buttons at runtime, as this is in fact what I am doing. Any chance you could suggest a method for doing so?Each button would need to be able to access the hashset yes? – cyo Feb 06 '20 at 22:22
  • For reference, just to test I changed Awake to Update and it works (of course, the scene slows to a grind). Perhaps I need to put the "// Run through all children GameObjects of this GameObject and check the tag" etc.. in a function and call then for each button when it is created? – cyo Feb 06 '20 at 22:51
  • No .. all you need to do when creating a button is also calling `childButtons.Add(theNewButton.gameObject);`. You can make it public so other components like the script that instantiates these buttons can access it and add the elements. Or you make a `public void AddButton(Button button){ childButtons.Add(button.gameObject);}` – derHugo Feb 07 '20 at 06:03
0

In Unity the Update function runs once every single frame!

So doing things like string comparisons in here might not be the best idea.

You might want to take user input in the Update and then do some calculations in the FixedUpdate which executes at a fixed frame rate.

That aside... In Unity there is a better way to do what you are doing by using CompareTag method.

This method gets compiled in a "special" way by the Unity compiler in order to avoid string allocations etc.. Basically is optimized for performance.

Here are some links that might help you clear the waters:

Performance of CompareTag

In this one do a search for CompareTag in your browser to see exactly your situation: Garbage Collection and other Optimizations

From Unity Forum

From JetBrains

Finally if you really need to compare two strings... The fastest way would most likely be like this:

string.CompareOrdinal(myString1, myString2) == 0 //this means equals


string.CompareOrdinal(myString1, myString2) != 0 //this means not equals
Jonathan Alfaro
  • 4,013
  • 3
  • 29
  • 32