Scene Import 1.0 #1034
Scene Import 1.0 #1034
Conversation
jturner65
requested review from
bigbike,
aclegg3,
Skylion007 and
eundersander
facebook-github-bot
added
the
CLA Signed
jturner65
force-pushed
the
MM_LoadSceneInstance_1.0
branch
from
23b4763
to
4836539
Compare
src/esp/sim/Simulator.h
Outdated
| * is created | ||
| * @return Whether successful or not | ||
| */ | ||
| bool createPathfinder(const std::string& navmeshFilename); |
There was a problem hiding this comment.
Wow, why is the necessary over the existing pathfinder API? shouldn't this be a method on pathfinder? Why should it be its own method. (There are already a lot of duplicate / unused methods on Simulator.h, so we should reuse / refactor when possible.)
There was a problem hiding this comment.
This is to support the eventual addition of multiple navmeshes, each of which will require their own pathfinder.
There was a problem hiding this comment.
Still should remove the amount stat calls to the file system. Doesn't pathfinder already do a file exists check?
There was a problem hiding this comment.
I agree that fewer OS hits would be a great thing. What you see was the existing mechanism for loading navmesh in reconfigure, which is why I have not modified it.
The loading function does check for file existence, along with multiple other failure conditions, and returns only a boolean success value. By having this test, we can know for certain whether failure is caused by non-existing file, or some other condition, without having to refactor PathFinder::loadNavMesh, which I felt is a bit beyond the scope of this PR.
|
Do we have any validate JSON functions, currently? I feel like those would be useful. |
jturner65
force-pushed
the
MM_LoadSceneInstance_1.0
branch
from
7a2ff02
to
f050a26
Compare
| // make sure that all stage, object and lighting attributes referenced in | ||
| // scene attributes are loaded in dataset, as well as the scene attributes | ||
| // itself. | ||
| datasetAttr->addNewSceneInstanceToDataset(sceneAttributes); |
There was a problem hiding this comment.
This says make sure, so can it fail? Should the process proceed if something fails to load here?
There was a problem hiding this comment.
That's what the function does. If it does not find named attributes within the dataset corresponding to those specified in the scene instance, it attempts to create them. This process will not "fail" per se, but empty results/unfound attributes may not be what the user wishes for. Or, it may be, depending on the source of the scene instance attributes that has been created.
| @@ -18,10 +18,28 @@ const std::map<std::string, esp::physics::MotionType> | |||
| SceneObjectInstanceAttributes::SceneObjectInstanceAttributes( | |||
| const std::string& handle) | |||
| : AbstractAttributes("SceneObjectInstanceAttributes", handle) { | |||
| setMotionType(-1); // defaults to unknown | |||
| // defaults to unknown/undefined | |||
| setMotionType(static_cast<int>(esp::physics::MotionType::UNDEFINED)); | |||
There was a problem hiding this comment.
What happens if you instance an object with "esp::physics::MotionType::UNDEFINED"? Does it just get the default of the current simulator implementation?
There was a problem hiding this comment.
Yes. This value in the scene object instance attributes is only used to override the default setting already in place. So this comes into play in the following code from Simulator.cpp (466) :
const physics::MotionType attrObjMotionType =
static_cast<physics::MotionType>(objInst->getMotionType());
const physics::MotionType objMotionType =
physicsManager_->getObjectMotionType(objID);
if ((attrObjMotionType != objMotionType) &&
(attrObjMotionType != physics::MotionType::UNDEFINED)) {
physicsManager_->setObjectMotionType(objID, attrObjMotionType);
}
There was a problem hiding this comment.
..which has been changed heh. Now it will only attempt to change the value if the object instance motion type value is not undefined.
// set object's motion type if different than set value
const physics::MotionType attrObjMotionType =
static_cast<physics::MotionType>(objInst->getMotionType());
if (attrObjMotionType != physics::MotionType::UNDEFINED) {
physicsManager_->setObjectMotionType(objID, attrObjMotionType);
}
src/esp/sim/Simulator.cpp
Outdated
| auto stageAttributes = metadataMediator_->getNamedStageAttributesCopy( | ||
| curSceneInstanceAttributes->getStageInstance()->getHandle()); | ||
| if (stageAttributes != nullptr) { | ||
| // assetType of stage is used to specify semanctic scene descriptor format |
There was a problem hiding this comment.
Is there no way to directly load what I configured without checking against the stage? If I specify a particular file in my Scene (e.g. a ".house") which does not correspond to the stage type defaults (e.g. an AssetType::INSTANCE_MESH stage which defaults to "info_semantic.json"), does this loading fail? I understand using the stage type when we are searching for the file initially during a back-compat phase, but if we specify the file in the configs we should just be able to load it, even if it isn't a typical type combination, right?
Use case: maybe I like a particular semantic scene format and manually make my own configs for object classes in that format.
There was a problem hiding this comment.
We need to come up with a better/expanded design for handling this, I agree. I would also prefer not to be tethered to the stage attributes for this - SSD loading should be completely independent of stage and stage type.
We could modify the check to use the file name given for the ssd, and if that fails to yield a usable type, then fall back on the scene instance. -shrug-.
There was a problem hiding this comment.
The mechanism has been modified somewhat. If an ssd filename is specified in the scene instance attributes, this is first checked for existence, and if it does exist, it is attempted to be loaded (currently loading is determined by its name). If what is specified does not load properly or is not found, then the previous implementation (pivoting on stageAttributes type) is executed. This is not the ideal mechanism, but is a reasonable compromise between the existing mechanism and leveraging the expanded capabilities of the scene instance.
We still need a mechanism by which the user can specify the format for the ssd file, or, better yet, develop a mechanism to intelligently load the appropriate file type using (meta)data in the file itself.
jturner65
force-pushed
the
MM_LoadSceneInstance_1.0
branch
6 times, most recently
from
6839496
to
f54916c
Compare
jturner65
force-pushed
the
MM_LoadSceneInstance_1.0
branch
5 times, most recently
from
a7c5f1e
to
f0efa36
Compare
…ade. If a scene instance is composed in blender, no notion of the COM will exist and any translations will need to be corrected by COM offset. However, in the future we plan on supporting the saving of existing, composed scenes to scene instance attributes, and the locations of objects will be already corrected for their COM offset. So this introduces a mechanism to differentiate between the two processes.
Remove the ability to set a stage attributes lighting key via json (this functionality has been supplanted by scene instances); Bypass misleading error on stage instancing.
jturner65
force-pushed
the
MM_LoadSceneInstance_1.0
branch
from
f0efa36
to
dd452db
Compare
| "template_name":"dataset_test_object2", | ||
| "translation":[0.3,0.4,0.5] | ||
| } | ||
| ], | ||
| "default_lighting":"modified_test_lights", | ||
| "frustum_culling":true, |
There was a problem hiding this comment.
I think we decided to remove this from config system and instead just keep SimulatorConfiguration as the entry point.
Motivation and Context
This PR replaces the old version found here since much of the old PR was split out into individual PRs that have been merged already.
This PR adds the support for importing scene instances. A scene instance describes a scene layout, using handles to attributes for the various assets used that are specified/defined in a scene dataset configuration file. The scene instance can describe the assets and transformations needed to build instances of a stage, 1 or more objects, a lighting configuration, navmesh and semantic scene descriptor files.
The stage and object instances described in a scene instance include their necessary transformations to align them properly in the scene. For objects, it also includes the motion type the object possesses, and whether or not the position of the placed object should be measured from the COM or from some asset-related origin.
All of Scene Instance Import functionality is driven by data specified in SimulatorConfiguration.
While this functionality is designed as part of the scene dataset configuration functionality that has been added, it is fully backwards compatible with the current scene/stage loading mechanism, and intended to remain so. If the user does not specify an existing scene dataset configuration, the system uses a default that can be modified and expanded (and in the next version of this functionality, saved as well). If the user attempts to load a stage via reconfigure, this functionality will internally construct a scene instance containing that stage and add it to the default dataset.
Future updates to this process are going to include the ability to save stage and object arrangements from within habitat-sim as scene instance configurations, as well as saving scene dataset configurations constructed/modified within habitat.
How Has This Been Tested
Types of changes
Checklist