isl-org / spear

SPEAR: A Simulator for Photorealistic Embodied AI Research
MIT License
224 stars 15 forks source link

update /Content/Kujiale #281

Closed rwang15 closed 4 months ago

rwang15 commented 4 months ago
mikeroberts3000 commented 4 months ago

This PR looks good. That being said, I'd like to review one more time before approving. I recognize that my recommended changes are minor and cosmetic, and therefore they don't really warrant another round of review. However, the last time I approved a PR with only minor cosmetic changes remaining, not all of those changes ended up in main. This is why I'd like to review one more time.

rwang15 commented 4 months ago
mikeroberts3000 commented 4 months ago
  • CG in MaterialFunctions
    • all funcions with CG are cherry picked from another plugin, and functions we added.

That makes sense.

  • renanming base material
    • it is a super expensive action where you need to refactor all of the material instance ever uploaded, and the change need to be done very carefully to not break any reference in Unreal. It could get very messy easily. We might need to find a good way to do if if we really want to rename them.
  • Renaming BP_Ceiling_Light_00 to BP_Ceiling_Light_0000 (also renaming the folder name)
    • it also requires changing the reference for all umaps at the same time to avoid broken reference. Fixing redirectory at the same time for all maps would cause Unreal to crash because it load too many asset at the same time. Im working on it but have some issue with unreal.

Fair enough. We can wait to pursue these improved naming conventions until we have a better sense of how to do it cleanly and safely, and how much effort it will be. I agree that it is not a good idea to attempt to load all Kujiale maps simultaneously and try to get the Unreal Editor to do it for us. We will need to write code that can do this for us automatically in a way that guarantees correctness, and can process one scene at a time.

That being said, I don't think we should wait indefinitely. If we can't afford to put effort into this now (or at least soon), how are we ever going to afford to put effort into this when we have an order of magnitude more scenes to deal with? I would like to revisit this topic after the ICRA demo.

It is clearly a confusing convention to have, e.g., BP_Ceiling_Light_00 to BP_Ceiling_Light_0000 in the same folder. And if we too liberally apply the argument that the existing conventions are too hard to change, it will paralyze us, and make it so we never improve things. Indeed, we could have applied this same argument to almost all of the improvements to our naming conventions we have made over the last 6-12 months. But if we did that, our data would still be a mess and would be very difficult to work with. I like how the legibility of our data has improved over time, and I'm glad we invested effort into improving it, even though it took a lot of work to do so. I don't want to slide backwards to how our data used to be, because we have decided that our conventions are too hard to change.

rwang15 commented 4 months ago

The references in umaps are all fixed except kujiale_0005 (there is some error opening the level). I fixed the redirectory per blueprint with the option fixup(keep redirectory).