Closed ahlamiki closed 1 year ago
The folder structure of this scene needs to be more carefully laid out and more consistent.
SM_Uppercase_First_Letters
for some assets and MI_lowercase_first_letters
for other assets. Make sure to "fix up redirectors" in the editor after renaming anything (https://docs.unrealengine.com/5.2/en-US/asset-redirectors-in-unreal-engine/).Blueprints
folder? The only thing that should be here is LightManager
, this asset should be named BP_LightManager
for consistency, and it should be in a Blueprints/Debug
folder.Maps
folder. There should not be any BuiltData
or any apartment_0001
data.Materials
. Rename LightTest
folder to Debug
. What does Masters
mean? What does each material in Materials
do? I don't understand the naming conventions here. You can organize the Materials
folder however you see fit, but the naming convention needs to be consistent and needs to make sense to me.Plataform
, Dinning
, Shelfs
.Base
and Clutter
rather than Building
and Props
.phong1
) stored in Meshes
, but other materials are stored in Materials
?Ceiling
folder in Clutter
(currently named Props
)? Shouldn't this be in Base
(currently named Building
) because that's where floor assets are located? Why are ceilings treated differently than floors? Likewise with Walls
.MI_fireplace
stored in Door
? Is this material used on doors? If so, why is it named MI_fireplace
?Human
folder?Kitchen
folder with some kitchen objects in it, but not a Living_Room
folder with living room objects in it? Why isn't Kitchen_chair
in the Kitchen
folder? The naming convention here is non-sensical. Reorganize to be consistent with the rest of the Clutter
folder (currently named Props
).Candleholders
have a dedicated folder, but Lamp_Corner
and Lamp_Corner_2
don't have a dedicated folder? I don't think these dedicated folders are useful.Picture
and Pictures
folders? Why are there Shelf
and Shelfs
folders (the correct spelling is Shelves
)?debug_0000
.Yes, I still have to do the clean up. I am working on it. Sorry for that.
Comment here when this PR is ready for me to review again.
Comment here when this PR is ready for me to review again. I am still checking the physic constraints but the scene has been improved and the clean up is done. You can check it again.
Here are some outstanding issues that I've already commented on but still are not fixed. Please carefully review all of my comments in this PR to make sure they're all addressed.
[/Script/Engine.PhysicsSettings]
since you probably did not intend to make any changes here.debug_0000
as the default startup map for internal development. We will make apartment_0000
the default map for external release builds, but that happens in a separate part of the code.debug_0000
.Here are some new comments:
MI_Kitchen_*
files outside the dedicated kitchen folder, so the usage of this folder is inconsistent. I don't think the kitchen folder adds that much to the organization here, because it doesn't contain very many files, it isn't consistent (e.g., there is no dedicated folder for living room), and all the material instances begin with the name of the object anyway (e.g., MI_KitchenSink_*
), so they are already sensibly grouped directly in the Material_Instances
folder without this additional layer of folder hierarchy. I think we should remove the kitchen folder and use your existing naming convention to organize the material instances directly in the Material_Instances
folder.M_
materials in the material instances folder.SM_Exctractor_doors
(also why is there a static mesh in the materials folder?)MI_Oven_doorGlass
, should be MI_Oven_DoorGlass
SM__SM_Extractor_*
, should be SM_Extractor_*
SM_kitchen_base
, should be SM_Kitchen_Base
(pick a capitalization convention and stick to it 100% of the time)Cabinet_Vase
is not in the Vases
folder? There are three tables, just like there are three vases, so why don't tables have a dedicated tables folder?Clean up the World Outliner. Use our design doc and the latest debug_0000
as a reference:
debug_0000
. This will require deciding on a semantic label for each actor. Use NYU40 semantic labels as described here (https://github.com/apple/ml-hypersim/blob/main/code/cpp/tools/scene_annotation_tool/semantic_label_descs.csv).SM_wall_*
aren't doing anything. Why are these actors here?GroupActor
into the Debug
folder.debug_0000
. Represent all PointLight_CeilingLamp_*
as PointLightComponents
on the corresponding StaticMeshActor
. Likewise for all the other lights that have static meshes associated with them. The reasoning for this approach is described in detail in our design doc.debug_0000
.PhysicsConstraintComponents
are not configured correctly. For example, the spatial location of Chest2.PhysicsConstraint_01
is not the same as the spatial location of Chest2.Door_01
, even though this is a requirement in our design doc. For now, it is fine if there are still simulation problems after the PhysicsConstraintComponents
are configured to adhere to all of our requirements, but I want to make sure that we're adhering to all the existing requirements before investing additional human effort into debugging simulation problems.SpotLight_CelingLamp_DinningRoom
SpotLight_KithecnLight_1
Minor nitpick: I think the drain in the sink is backwards. It should be directly under the tap, instead of being close to the edge of the counter.
I'll review the project structure in the World Outliner on Monday. In the meantime, there is a bit of basic cleanup to do.
I get the following warnings when I build and cook via the command-line on Windows:
LogMaterial: Warning: [AssetLog] F:\code\github\spear\cpp\unreal_projects\SpearSim\Content\Scenes\apartment_0000\Materials\Materials\M_Master_Opacity.uasset: Failed to compile Material for platform PCD3D_SM6, Default Material will be used in game.
(Node TextureSampleParameter2D) Sampler type is Color, should be Linear Color for /Game/Scenes/apartment_0000/Meshes/Clutter/Fireplace/T_Fireplace_orm.T_Fireplace_orm
(Node TextureSampleParameter2D) Sampler type is Color, should be Linear Color for /Game/Scenes/apartment_0000/Meshes/Clutter/Fireplace/T_Fireplace_orm.T_Fireplace_orm
LogMaterial: Warning: [AssetLog] F:\code\github\spear\cpp\unreal_projects\SpearSim\Content\Scenes\apartment_0000\Materials\Materials\M_Master_Opacity.uasset: Failed to compile Material for platform PCD3D_SM5, Default Material will be used in game.
(Node TextureSampleParameter2D) Sampler type is Color, should be Linear Color for /Game/Scenes/apartment_0000/Meshes/Clutter/Fireplace/T_Fireplace_orm.T_Fireplace_orm
(Node TextureSampleParameter2D) Sampler type is Color, should be Linear Color for /Game/Scenes/apartment_0000/Meshes/Clutter/Fireplace/T_Fireplace_orm.T_Fireplace_orm
LogMaterial: Warning: Cooking a material resource (in M_Master_Opacity hierarchy) that doesn't have a valid ShaderMap! Shadermap pointer is null.
LogMaterial: Warning: Cooking a material resource (in M_Master_Opacity hierarchy) that doesn't have a valid ShaderMap! Shadermap pointer is null.
LogUObjectGlobals: Warning: [AssetLog] F:\code\github\spear\cpp\unreal_projects\SpearSim\Content\Scenes\apartment_0000\Materials\Material_Instances\MI_Curtains_01.uasset: Failed to load '/Game/Scenes/apartment_0000/Meshes/Clutter/Curtains/T_Curtain_Height': Can't find file.
LogLinker: Warning: [AssetLog] F:\code\github\spear\cpp\unreal_projects\SpearSim\Content\Scenes\apartment_0000\Materials\Material_Instances\MI_Curtains_01.uasset: VerifyImport: Failed to load package for import object 'Package /Game/Scenes/apartment_0000/Meshes/Clutter/Curtains/T_Curtain_Height'
Please make sure that the project builds and cooks without warnings on the command-line.
Lighting looks weird when I open the latest project in the editor:
Lighting still looks weird for me in the latest project.
In my last version of the scene (which I uploaded to sourcetree) I fixed up all the redirectors in apartment_0000 and the file "MI_Table_Living" has been changed to "MI_LivingRoom_Table".
De: Mike Roberts @.> Enviado: martes, 27 de junio de 2023 23:39 Para: isl-org/spear @.> Cc: Ahlam @.>; Author @.> Asunto: Re: [isl-org/spear] Adding apartment_0000 (PR #205)
@mikeroberts3000 commented on this pull request.
On cpp/unreal_projects/SpearSim/Content/Scenes/apartment_0000/Materials/Material_Instances/MI_Table_Living.uassethttps://github.com/isl-org/spear/pull/205#discussion_r1244390992:
Please "fix up redirectors" in the editor for all redirectors if you haven't already. And please review your changes in the "Files changed" tab on GitHub as well as in the Unreal Editor so you can catch similar issues in future.
— Reply to this email directly, view it on GitHubhttps://github.com/isl-org/spear/pull/205#discussion_r1244390992, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A234SOYLBSFNF2QYP5GFDZTXNNHJTANCNFSM6AAAAAAY6JU7LQ. You are receiving this because you authored the thread.Message ID: @.***>
Lighting still looks weird for me in the latest project.
![Screenshot (28)](https://user-images.githubusercontent.com/2341965/249287511-dd9592ed-96a8-4405-bd4e-acfa21084edb.p I think the HDRI is making this problem, I'm finding out what happen with this file.
After reading your latest comments in the COMMENTS.pdf
file you sent me via Discord, I have the following follow-up comments:
Materials/Materials
and Materials/Material_Instances
look good to me.Meshes/*
directly into Materials/Textures
with no additional subfolders. It is confusing that textures are currently stored in a folder called Meshes
. And there are textures that apply to multiple meshes (e.g., T_Cabinet_Vase_Mirror
), so it is confusing to store a shared texture with a particular mesh (e.g., storing a shared texture with Cabinet
but not with Vase
or Mirror
).The following comments apply to the Outliner view:
Composite
folder into the Meshes
folder (see below for details).Meshes
folder, create subfolders using the following semantic category names and IDs, and move all relevant actors into these subfolders:
01_wall
, 02_floor
, 03_cabinet
, etc.Rendering/Lights/Ceiling_Kitchen_Lights
and Rendering/Lights/Ceiling_LivingRoom_Lights
to the Meshes/35_lamp
folder. We want these actors to be stored in Meshes
because they each have a mesh attached to them, and we want them to be in the 35_lamp
folder because lamp
is the closest semantic category to an overhead light.Rendering/Lights/HDRIBackdrop
to Rendering/HDRI/HDRIBackdrop
. I don't have a definitive justification for this, but I subjectively believe that HDRI actors deserve their own subfolder.Static
. We should set the mobility of all lights to Movable
so they interact correctly with Lumen.The following comments apply to the World Settings view:
After reading your latest comments in the
COMMENTS.pdf
file you sent me via Discord, I have the following follow-up comments:
Materials/Materials
andMaterials/Material_Instances
look good to me.- Let's move the textures in
Meshes/*
directly intoMaterials/Textures
with no additional subfolders. It is confusing that textures are currently stored in a folder calledMeshes
. And there are textures that apply to multiple meshes (e.g.,T_Cabinet_Vase_Mirror
), so it is confusing to store a shared texture with a particular mesh (e.g., storing a shared texture withCabinet
but not withVase
orMirror
).
It has been changed.
The following comments apply to the Outliner view:
- Move all the actors in the
Composite
folder into theMeshes
folder (see below for details).In the
Meshes
folder, create subfolders using the following semantic category names and IDs, and move all relevant actors into these subfolders:
- https://github.com/apple/ml-hypersim/blob/main/code/cpp/tools/scene_annotation_tool/semantic_label_descs.csv
- These semantic categories aren't perfect, but lots of people in the robotics community are already using them.
- For example, create subfolders like
01_wall
,02_floor
,03_cabinet
, etc.- Only create a subfolder if you intend to place at least one actor in it.
- Move
Rendering/Lights/Ceiling_Kitchen_Lights
andRendering/Lights/Ceiling_LivingRoom_Lights
to theMeshes/35_lamp
folder. We want these actors to be stored inMeshes
because they each have a mesh attached to them, and we want them to be in the35_lamp
folder becauselamp
is the closest semantic category to an overhead light.- Move
Rendering/Lights/HDRIBackdrop
toRendering/HDRI/HDRIBackdrop
. I don't have a definitive justification for this, but I subjectively believe that HDRI actors deserve their own subfolder.- There are some lights that cause the word "Preview" to be projected onto the scene in the editor viewport. This happens because their mobility is set to
Static
. We should set the mobility of all lights toMovable
so they interact correctly with Lumen.- Otherwise the structure in the Outliner is good.
The following comments apply to the World Settings view:
- There are some non-default values in the World Settings view. We should avoid changing things in World Settings unless we have a good reason.
After following the semantic category names of each subfolder of "Meshes", should I change the naming structure of each SM from e.g. "Big_Carpet" to "bigcarpet" or "big_carpet" ?
The comments regarding to the Outliner have been fixed.
After following the semantic category names of each subfolder of "Meshes", should I change the naming structure of each SM from e.g. "Big_Carpet" to "bigcarpet" or "big_carpet" ?
The comments regarding to the Outliner have been fixed.
There is no need to rename the stuff in the Content Browser. Your existing naming convention in the Content Browser is fine. The category subfolders in the Outliner are just to provide a bit of additional organization, they don't need to match the naming conventions and hierarchy in the Content Browser and on disk. Please use the semantic categories and integer IDs from the link I provided.
It is good that the lighting works correctly now. There are still some outstanding questions and cleanup items. Please address all my outstanding comments.
Adding apartment_0000. This scene uses the UE5 latest features, lumen illumination and it has several assets and textures made from scratch for the development of a high quality apartment. In addition to that, I implemented physics constraints to the articulable parts of the furniture in order to be fully functional as a default scene.