Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RGL object fetching reworked #312

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RGL object fetching reworked #312

wants to merge 1 commit into from

Conversation

pijaro
Copy link
Contributor

@pijaro pijaro commented Jun 7, 2024

The approach for the system looks as follows:

  • Fetch all the scene objects on the initialization, only once,
  • Each object spawned on the scene (vehicle or pedestrian) registers itself in the RGL SceneManager.
  • Each object despawned from the scene (vehicle or pedestrian) unregisters itself in from the RGL SceneManager.

In a result, there are no frequent fetches of the whole level (FindObjectsOfType).

Requires #148

Signed-off-by: Piotr Jaroszek <[email protected]>
@mackierx111 mackierx111 marked this pull request as draft June 11, 2024 09:05
@mackierx111
Copy link
Collaborator

Change to draft until #148 is merged

@mackierx111 mackierx111 self-requested a review June 19, 2024 14:59
@mackierx111 mackierx111 marked this pull request as ready for review June 19, 2024 15:00

void Start()
{
sceneManager = FindObjectOfType<RGLUnityPlugin.SceneManager>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think each NPC class should reference the RGL SceneManager. Is there a problem if the call to SceneManager.RegisterRGL is called by the class that spawns the NPC?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mackierx111 CC @pijaro

I apologize for the late response. I see one main issue with this approach, which is that for what I know, NPCPedestrian.cs does not have a spawning class when AWSIM is run without SSv2. For example in Nishinjuku scene, the pedestrian GameObjects are already present in the scene.

image

This of course can be resolved by having an additional script in the scene, which is given references to all pedestrian GameObjects and is responsible for registering them with RGL. Do you want us to implement such a solution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I apologize for the late response.

no problem!

In any case, I don't think we should add code that depends on other classes. If such additions are made in the future, there will be more and more code to add caches other than RGL to each component class, such as NPCs, which we do not want.

For example, the following workarounds can be suggested

  • Have NPC Pedestrian spawn at the start of Simulation. (And the spawning class will do the RGL cache)
  • Refer to the first placed NPC Pedestrian and add it to the RGL cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood, we will implement changes not to have references to RGL within pedestrian classes.

@ceccocats
Copy link

ceccocats commented Jun 28, 2024

Nice work 👍
Unfortunately SceneManager: Update transforms and skinned meshes is still slow. It would be possible to just update objects that are not marked static in unity?

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.

4 participants