3drepo / 3drepo.io

3D Repo web server
http://www.3drepo.io
GNU Affero General Public License v3.0
95 stars 38 forks source link

Isolate model from tree card error #4977

Closed Charence closed 2 weeks ago

Charence commented 5 months ago

Description

Issue: In federation if we try to isolate one model from Tree card, its giving below error and 3DRepo restarting.

Steps to replicate

Current Behaviour

Expected Behaviour

Pictures/ Gifs of the error

image

carmenfan commented 5 months ago

This is actually to do with the Zoom to object function (for the tree isolation feature, we do both isolate and zoom to the isolated objects)

When the action is done on a object that has too many meshes, it can produce a massive message for unity to absorb, which may make unity go out of memory (https://forum.unity.com/threads/max-length-for-the-string-param-in-sendmessage.13579/)

We mitigate that with show/hide/isolate by splitting the request to multiple requests. But for zoom we would need to know all the objects we zoom to (otherwise the camera may be moved to an incorrect place)

So this solution wouldn't work. and in general is an issue for the zoom to object feature.

sebjf commented 5 months ago

@FJThiel, can you take a look at this next?

(Though the ticket is in .io, it's a bug in the interface between Unity and Js.)

The first thing to check is how many methods there are like ZoomToObjects - i.e. ones that take a list of submesh ids, but don't use multipleCallInChunks, in UnityUtil.

If, as I suspect, its just this one, then the most straightforward solution is going to be to make it so Unity's ZoomToObjects supports calling over multiple frames. For example, we have two methods ZoomToObjectsAppend and ZoomToObjectsEnd, and the former can be called multiple times to build a list that the latter then acts upon.

UnityUtil still exposes zoomToObjects to the frontend, but internally uses those two Unity methods to split the arguments list with multipleCallInChunks like the others.

If it turns out there are many such instances of this, then we could also look at other mechanisms to provide long lists in arguments.

FJThiel commented 5 months ago

@sebjf, I just checked UnityUtil and there are indeed two methods with that behaviour. In addition to zoomToObjects, there is also centreToPoint. It takes an array of json objects and moves the pivot point to the centre of the objects provided.

sebjf commented 5 months ago

@FJThiel, great, in that case lets just update those two methods so they can be invoked across multiple calls.

FJThiel commented 5 months ago

@sebjf, quick logistical question: If I understand the proposed solution correctly, the behaviour needs to be implemented across two repositories: The two new methods on the Unity side and the splitting of the arguments across these two in io. How do you want this to look in terms of branches? Two ISSUE_4977 branches, with one in io and one in 3drepounity or is there some other mechanism/convention that I should use?

sebjf commented 5 months ago

create a new issue in 3drepounity for the unity changes, and make the .io ones in 4977. (Build the unity viewer into the .io one and commit for testing - Carmen will rebuild the actual one when everything gets merged into staging!)