open-rmf / rmf_site

Experimental visualizer for dense buildings in RMF
33 stars 13 forks source link

[Bug]: Invalid asset source causes models to disappear without a way to retrieve them #241

Open xiyuoh opened 2 weeks ago

xiyuoh commented 2 weeks ago

Before proceeding, is there an existing issue or discussion for this?

OS and version

Ubuntu 24.04

Open-RMF installation type

Source build

Other Open-RMF installation methods

No response

Open-RMF version or commit hash

main

ROS distribution

Jazzy

ROS installation type

Docker

Other ROS installation methods

No response

Package or library, if applicable

No response

Description of the bug

When modifying the asset source, if an invalid source is provided, the model will disappear while the entity still exists. This makes it impossible for users to click on the same model to input a valid asset source.

Steps to reproduce the bug

  1. Open demo map, select any model.
  2. Under the Inspect widget, with Source set to Search, type in any invalid model path. For example, OpenRobotics/ or leave it empty. Press Enter.

Expected behavior

A suggested behavior would be for the model to remain visible with the previous valid asset source until a new valid asset source is provided.

Actual behavior

Model disappears with no way to retrieve it, warnings printed about component mismtach and Model not found. The asset source can no longer be modified from the editor as the model is hidden and cannot be selected.

Additional information or screenshots

From an offline discussion with @mxgrey I understand that asset loading behavior can be greatly improved with the workflows PR https://github.com/open-rmf/rmf_site/pull/238, opening this ticket to track the progress of this bug.

luca-della-vedova commented 2 weeks ago

The main worry I have with this behavior is that the state of the site won't actually represent what the value of the components have. The main case is that if users were to change the value of a model to something invalid, the model was still spawned, they saved then reloaded the file, the model would disappear, so the error can only be found out after reloading the site at which point it might be too late. By contrast in the current approach users would have immediate feedback that the proposed AssetSource does not work and they can still edit it while the model is selected but with no spawned mesh (unideal yes, in case they click something else, but from what I understand a lot of this would be addressed by the scenario work?)

xiyuoh commented 2 weeks ago

I wonder if it makes sense to reject invalid inputs to preserve what's reflected on the site to be consistent with the components. So the value is not changed until we verify that it is valid.

By contrast in the current approach users would have immediate feedback that the proposed AssetSource does not work and they can still edit it while the model is selected but with no spawned mesh (unideal yes, in case they click something else, but from what I understand a lot of this would be addressed by the scenario work?)

Unfortunately if users take a longer time to type, the Inspect widget would close and the model is lost. I tried leaving the partial asset value untouched for a couple seconds and visibility is automatically updated with the invalid source. But you're right that the scenarios PR would fix that such that we can always access the asset source.

With the scenarios PR we can select the Model Description (they are still available and listed under Groups even if Model Instances are invisible) and try to correct the asset source, but after an invalid value is passed, even if we re-enter a valid one the Model Instances remain hidden. Added a video below to illustrate.

https://github.com/user-attachments/assets/4d7d17bb-8a1b-43d2-bad0-941dc9bf60ea

mxgrey commented 2 weeks ago

The main case is that if users were to change the value of a model to something invalid, the model was still spawned, they saved then reloaded the file, the model would disappear, so the error can only be found out after reloading the site at which point it might be too late.

Good points, I would propose the following behavior:

  1. When a model loading error happens, we retain the last successfully loaded model but give it a permanent red outline using the existing outlining pipeline. The red outline remains until the asset source is changed to something valid.
  2. If a valid model has never been loaded, we fall back on some builtin placeholder model. A fallback commonly used by game engines is a square block with a ? or ! texture on all six sides.

I think once we've finished migrating model spawning to use workflows this behavior should be pretty reasonable to pull off.

mxgrey commented 2 weeks ago

We should probably also have a way for the issue reporting widget to list models with invalid asset sources and allow the user to select those items and fix the asset sources.