1
\$\begingroup\$

I have to load/unload models from a specific URL based on player position. For this reason I am checking player position from model and then load/unload a related piece of model in an Update event which runs on every frame.

Here is the update that validates some checks before loading/unloading. I added this check for optimization purposes as the main loading/unloading loop is heavy:

private void Update()
{
    //If this feature is disable don't load/unload
    if (enableThisFeature == false) return;

    //if player is moving, dont load//unload
    if (Input.GetAxis("Horizontal") != 0 || Input.GetAxis("Vertical") != 0)
    {
        //do nothing when player is moving
        return;
    }

    DeactivateDistantTiles();
}

After this I am checking player postion and calling model load/unload:

private void DeactivateDistantTiles()
{
    playerPosition = transform.position;
    playerPosition = cameraController.currentActiveCam.transform.position; //transform.position;

    checkPlayerPositionChangeing = playerPosition.z != playerLastPos.z || playerPosition.x != playerLastPos.x;

    if (checkPlayerPositionChangeing)
    {
        ABLoadUnloadLoopCall();
    }


    playerLastPos = cameraController.currentActiveCam.transform.position;

}
Vector3 tilePosition;
float xDistance;
float zDistance;
public void ABLoadUnloadLoopCall()
{

      //old
        //foreach (SingleABLoader tile in tiles)
        //{
        //    Debug.Log("ABLoadUnloadLoopCall 123");
        //    Vector3 tilePosition = tile.gameObject.transform.position + (tileSize / 2f);

        //    float xDistance = Mathf.Abs(tilePosition.x - playerPosition.x);
        //    float zDistance = Mathf.Abs(tilePosition.z - playerPosition.z);

        //    if (xDistance + zDistance > maxDistance)
        //    {
        //        /*If you don't want to destroy the object on unload then use below line otherwise use DestroyBundleObject with true pararmeter */
        //        //tile.DestroyBundleObject();
        //        tile.DestroyBundleObject(true);
        //    }
        //    else
        //    {
        //        tile.StartDownloadingAB();
        //    }

        //}
        //new
        for(int i = 0; i < tiles.Length; i++)
        {

            tilePosition = tiles[i].gameObject.transform.position + (tileSize / 2f);

             xDistance = Mathf.Abs(tilePosition.x - playerPosition.x);
             zDistance = Mathf.Abs(tilePosition.z - playerPosition.z);

            if (xDistance + zDistance > maxDistance)
            {

                /*If you don't want to destroy the object on unload then use below line otherwise use DestroyBundleObject with true pararmeter */
                //tiles[i].DestroyBundleObject();
                tiles[i].DestroyBundleObject(true);

            }
            else
            {
                tiles[i].StartDownloadingAB();
            }

        }


}

I found that ABLoadUnloadLoopCall making GC allocations in KBs is very high. Is there any way available that my code can be optimized and make fewer allocations? My initial research suggest to use a for loop instead of a foreach, therefore in ABLoadUnloadLoopCall I am using a for loop instead of foreach but my game still lags/freezes for some minutes after loading the model.

Here is the Bundle/Model loading code:

public IEnumerator DownloadAB()
{

    if (isBundleLoading == true)
        yield return false;

    BundleLoadStatus = BundleLoadStatusEnum.bundlesLoading;
    isBundleLoading = true;

    www = UnityWebRequestAssetBundle.GetAssetBundle(finalABLoaderURL);
    yield return www.SendWebRequest();

    if (www.error != null)
    {
        Debug.LogError("assetBundleURL : " + finalABLoaderURL);
        Debug.LogError("www error : " + www.error);
        www.Dispose();
        www = null;
        yield break;
    }

    bundle = ((DownloadHandlerAssetBundle)www.downloadHandler).assetBundle;
    //GameObject bundlePrefab = null;
    //bundlePrefab = (GameObject)bundle.LoadAsset(bundle.name);

    //bundlePrefab = (GameObject)bundle.LoadAsset(bundle.GetAllAssetNames()[0]);
    AssetBundleRequest bundlePrefabAsync = bundle.LoadAssetAsync(bundle.name, typeof(GameObject));
    //yield return bundlePrefab;
    yield return bundlePrefabAsync;

    // if we got something out
    if (bundlePrefabAsync != null)
        //if (bundlePrefab != null)
    {
        //First Off the Origin S***fting
        environmentOriginSetter.EnvironmentOriginSetterFeatureActive(false);//TODO
        //assetBundleToLoadObj = (GameObject)Instantiate(bundlePrefab);

        //Then Instantiate the Bundel Object and make it child to environment parent object.
        assetBundleToLoadObj = Instantiate(bundlePrefabAsync.asset as GameObject);
        assetBundleToLoadObj.transform.parent = envParent.transform;
        //assetBundleToLoadObj.transform.parent.transform.position = this.transform.localPosition;//new

        //Then Enable the Origin Setter feature again
        environmentOriginSetter.EnvironmentOriginSetterFeatureActive(true);

        //disable the floor L7 Mesh
        floorL7MeshRenderer.enabled = false;//TODO
    }

    www.Dispose();
    www = null;

    // try to cleanup memory
    //Resources.UnloadUnusedAssets();//TODO open if memory problem occur
    bundle.Unload(false);//TODO open if memory problem occur
    bundle = null;

    isBundleLoading = false;
    BundleLoadStatus = BundleLoadStatusEnum.bundlesHasLoaded;
}

For Unloading, I am destroying the object:

 public void DestroyBundleObject(bool isDestroy)
{
    //bundle was loaded completely, write a bool, if it is true then loaded completely
    //bundle is loading, isBundleLoading
    //bundle is not load yet, 

    if (bundleObjectsDeleted == false && BundleLoadStatus == BundleLoadStatusEnum.bundlesHasLoaded)
    {
        //BundleObjectActive(false);
        bundleObjectsDeleted = true;
        if (assetBundleToLoadObj)
        {
            Destroy(assetBundleToLoadObj);
        }

        //bundle.Unload(true);
        //bundle = null;

        BundleLoadStatus = BundleLoadStatusEnum.bundleNotLoadedYet;
        //Resources.UnloadUnusedAssets();//TODO Open it.
    }

}

Before loading the bundle I Enque the bundle in a list and download it one by one:

public void EnqueABDownloading(SingleABLoader abToDownload)
{

    if (!singleAbLoader.Contains(abToDownload))
    {
        Debug.Log("Enque " + abToDownload.gameObject.name);
        singleAbLoader.Enqueue(abToDownload);

        if (isDownloadStarted == false)
        {
            StartCoroutine(StartDownloading());
        }
    }

}

public IEnumerator StartDownloading()
{

    isDownloadStarted = true;
    Application.runInBackground = true;//enforce background loading.
    imgBlockClicks.SetActive(true);
    textLoading.SetActive(true);
    loadingSlider.gameObject.SetActive(true);

    while (singleAbLoader.Count > 0)
    {
        float sliderIncrementValue = 1f / singleAbLoader.Count;

        SingleABLoader singleAbLoaderCurrent = singleAbLoader.Dequeue();
        Debug.Log("Starting to call " + singleAbLoaderCurrent.gameObject.name);
        yield return singleAbLoaderCurrent.DownloadAB();
        Debug.Log("Finsihed to call " + singleAbLoaderCurrent.gameObject.name);
        //Debug.Log("Finished next loop");
        //singleAbLoaderCurrent.CallDownloadAB();
        //Debug.Log("download call for "+ singleAbLoaderCurrent.name);
        loadingSlider.value = sliderIncrementValue;
    }

    isDownloadStarted = false;

    textLoading.SetActive(false);
    imgBlockClicks.SetActive(false);

    //Application.runInBackground = false;
    loadingSlider.gameObject.SetActive(false);
    loadingSlider.value = 0;

}

I tried to profile on editor and found something like this:

enter image description here

\$\endgroup\$

closed as off-topic by 1201ProgramAlarm, yuri, Pieter Witvoet, esote, pacmaninbw Jun 2 at 15:44

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – 1201ProgramAlarm, yuri, Pieter Witvoet, esote, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Is this a code review, or a question of how to do something? \$\endgroup\$ – D. Ben Knoble May 30 at 2:21
  • \$\begingroup\$ Its code review Ben! I want to optimzie the given code \$\endgroup\$ – Muhammad Faizan Khan May 30 at 6:59
  • \$\begingroup\$ Replacing a foreach loop with a for loop will rarely make a difference - it's what you're doing inside of that loop that matters. But there's very little we can say about that without knowing roughly how many tiles there are, what they contain and what those DestroyBundleObject and StartDownloadingAB methods do. \$\endgroup\$ – Pieter Witvoet May 30 at 10:13
  • \$\begingroup\$ Also, it's not clear what the actual problem is: you mention 'GC alloc' (?), but loading a model into memory obviously involves memory allocation. You also mention 'lag/freeze for some minutes', but does that mean that the game freezes for several minutes (which would happen if you download a model synchronously), or does it lag for several minutes (as in occasional drops in fps, which might be caused by heavy GC pressure)? \$\endgroup\$ – Pieter Witvoet May 30 at 10:17
  • 1
    \$\begingroup\$ At this point the question is more 'how can I solve this problem' than 'how can I improve this code', and as such I don't think it belongs here on Code Review. You're probably better off asking this on a Unity-specific forum. \$\endgroup\$ – Pieter Witvoet May 31 at 8:45
2
\$\begingroup\$

Add hysteresis to the unload. That way the unload doesn't happen immediately when the player move out of range where if he dances along the boundary line he could a lot of loading and unloading otherwise.

if (xDistance + zDistance > unloadDistance)
{
    tiles[i].DestroyBundleObject(true);
}
if (xDistance + zDistance < startLoadDistance)
{
    tiles[i].StartDownloadingAB();
}

Also ensure that StartDownloadingAB() can early out and do nothing when the tile is already loaded.

If memory becomes tight you can add the player velocity to the distance logic to prioritize the tiles ahead over the tiles to the side.

\$\endgroup\$
  • \$\begingroup\$ Thanks I will check that but do u think that it will be impactful? \$\endgroup\$ – Muhammad Faizan Khan May 29 at 15:44
  • \$\begingroup\$ Thanks for recommendation but it didn't make any performance impact. \$\endgroup\$ – Muhammad Faizan Khan May 30 at 9:52

Not the answer you're looking for? Browse other questions tagged or ask your own question.