overte-org / overte

Overte open source virtual worlds platform.
https://overte.org/
Other
132 stars 48 forks source link

Entity stays invisible when deleting Zone #795

Open JulianGro opened 5 months ago

JulianGro commented 5 months ago

When you delete the zone that an entity is using as part of "Render With Zones", the entity will stay hidden and display an "ERROR: NOT FOUND". The error seems nice, but I would argue that an entity should not be culled if the zone that culls it does not exist. IMG_20240123_110519

AleziaKurdis commented 5 months ago

This is not a bug, it is the expected behavior. If we change this, it would make the system glitchy. A zone can be small and distant and because of that might not be loaded. this would start to show content that should not be, wasting the optimization that was aimed.

You need to manage the parenting if you delete the zone. The create app support multiple entity selection. so it is easy to address rapidly.

Also revealing imply to know what zone is a broken link. Doing this would have a performance cost. (@HifiExperiments could confirm or infirm this.)

gydence commented 5 months ago

yeah I agree that this is the correct behavior. maybe when you delete a zone, its ID should be removed from all renderWithZones lists? but we don’t currently keep track of that mapping anywhere and it would be a bit expensive to loop over all entities. we’d have to figure out where to put that logic…create or interface could not have loaded entities yet or might not have edit permission, so I guess it would be on the entity server

AleziaKurdis commented 5 months ago

There are other reasons to obtain a broken link by the way. (Copy of the entity in a different domain, import a json that has an id but with no zone )

Most of the time I prefer to have a clue that there was a broken link (in order to manage it) If you delete a zone by accident you still have at least a clue of which entities were depending on it.

I would say: Caution with changing this behavior for something destructive. It is probably not top priority to address anyway. Maybe we could wait a 10th user to complain, since we are like 3 people having use this feature. (yes I'm certainly exegerating... but not that much )

gydence commented 5 months ago

fair 😁

I think the entity list can show you when URLs are broken, right? maybe it could also show a warning for this

AleziaKurdis commented 5 months ago

We could have this kind of thing yes. it would be useful. So more an enhancement anyway

AleziaKurdis commented 5 months ago

Maybe a tool specifically to manage the broken links. It would list the id that are not resovled among the renderWithZones properties and offer the choice to: Remove or Replace them. It would work better than the multiple selection. and control the list of the entities that you want to fix,

AleziaKurdis commented 4 months ago

I suggest we close this now.

JulianGro commented 4 months ago

This doesn't fix the original issue as far as I can tell. I don't see why we cannot just run a check when deleting a zone via the Create App to check if something gets culled by the non-existent zone.

AleziaKurdis commented 4 months ago

The find entity can’t be trusted for that, it might not not return entities that are far and small and never been visited in the current session. So such warning might fail to advise you and this entity will stay hidden anyway.

Also there is the case where you paste entities that come from another domain and have a value in renderWithZones. Same issue here. We can’t trust the Find entities, so zone zone might be wrongly considered as missing. So you might get the no warning and still get the pasted entities not visible.

JulianGro commented 4 months ago

If Create App itself cannot do it, maybe we should have the server deal with it. My understanding is that we just need to have the server run the search for entities. Since people who can add and remove zones already have Create permissions, their client should be able to send commands to the server.

If our permission system becomes more fine grained in the future, we could add a special function for that; I think @74hc595 has done something similar here: https://github.com/overte-org/overte/pull/664

AleziaKurdis commented 4 months ago

yeah... but create app should also work for serverless. It sound like complicated for a usecase that seem questionable. Can you explain what was the context when you experienced that? Was that a test of a real case ?

HifiExperiments commented 4 months ago

I would vote for doing this one the create side rather than the server so that we don't have to worry about the permissions issues (and so that this will work in serverless, great point). I agree that findEntities might miss an entity, or that the client might not have permissions to modify the other entities, but those cases should be rare and the new RenderWithZones manager makes it easy to resolve.

separately, for the pasting issue, renderWithZones IDs should get re-written when pasting entities so that they target the new ID. if that's not working as intended we should fix it

AleziaKurdis commented 4 months ago

separately, for the pasting issue, renderWithZones IDs should get re-written when pasting entities so that they target the new ID. if that's not working as intended we should fix it

I undertsand that you will continue to set the id in renderwithzone only if the zone is part of the copy? and of course discard it when it is not, right?

HifiExperiments commented 4 months ago

I think that's how it's supposed to work 😜