merged

Extend yaml loading

Sent by Harald Scheirich on 2015-01-21 23:09 (over 9 years ago) Merged by: Harald Scheirich on 2015-02-14 00:57 (about 9 years ago)

This adds two function to the scene loading in runtime, addSceneElements() and cloneSceneElements(), these functions make the data more useful as it not does not need to contain all of the scene description. It also lets us repeatedly clone a scene element like a staple to reuse it over and over

Version: 6

by Harald Scheirich on 2015-02-14 00:57 (about 9 years ago)

+0 -0

From: OpenSurgSim:feature/add-scene(eb025e3b9b)
To: OpenSurgSim:master (76ba9776bc)
Merged: 0cf9b4b390

Discussion 0
Commits -
Files Affected -
Tickets Affected -
Followers -

show votes

Version: 5

by Harald Scheirich on 2015-02-14 00:33 (about 9 years ago)

+0 -0

From: OpenSurgSim:feature/add-scene(7af6063d1d)
To: OpenSurgSim:master (488529a201)

Discussion 0
Commits -
Files Affected -
Tickets Affected -
Followers -

show votes

Version: 4

by Harald Scheirich on 2015-01-31 02:16 (about 9 years ago)

13|

+3 -0

From: OpenSurgSim:feature/add-scene(2c75151e80)
To: OpenSurgSim:master (4f02f8f64e)

Discussion 13
Commits -
Files Affected -
Tickets Affected -
Followers -

Show code comments

on 2015-01-31 02:16 By Harald Scheirich

Fixed lInux errors

on 2015-02-05 23:40 By pnovotny

@jlenoir @ryanbeasley @wdturner @hscheirich should we discuss this request?

on 2015-02-06 01:09 By Harald Scheirich

@pnovotny I thought this was done ?

on 2015-02-12 23:33 By pnovotny view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

result = true;

Can you bring these asserts and logging in agreement between these two try* functions (and Asset load)? Maybe only log in WARN/SEVERE in tryLoadNode and tryConvertElements and return false. Then add an assert in the calling function if you want.

on 2015-02-13 19:30 By Harald Scheirich view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

result = true;

@pnovotny I actually really don't, i would rather get rid of the bool result than the assert, starting to warn at a lower level means that we will have to check the return results at the higher levels and still assert, which overall will reduce the stabilty in the system because now there are two if's that will have to be implemented for each error rather than one at the original point of failure. This is, while we have a lot of asserts, what makes OSS stable, because failure can't propagate up the hierarchy. If they could you now force the rest of the system to implement code to actually handle those failures. You are right though that within this failure is handled inconsistenly, i'll remedy that.

on 2015-02-12 23:34 By pnovotny view inline at SurgSim/Framework/Runtime.h (merge request Extend yaml loading)

bool tryConvertElements(const std::string& fileName, const YAML::Node& node,

yeah, this "return" comment suggests it won't assert, but return false if unsuccessfull.

std::vector<std::shared_ptr<SceneElement>>* elements);

on 2015-02-13 19:31 By Harald Scheirich view inline at SurgSim/Framework/Runtime.h (merge request Extend yaml loading)

196	bool tryConvertElements(const std::string& fileName, const YAML::Node& node,
	@pnovotny will refactor this to assert in all cases
197	std::vector<std::shared_ptr<SceneElement>>* elements);

on 2015-02-12 23:34 By pnovotny view inline at SurgSim/Framework/Scene.cpp (merge request Extend yaml loading)

69	for (auto element : elements)
	auto& ?
70	{

on 2015-02-12 23:35 By pnovotny view inline at SurgSim/Framework/Scene.cpp (merge request Extend yaml loading)

69	for (auto element : elements)
	oh, they are just shared_ptr's. never mind.
70	{

on 2015-02-12 23:38 By pnovotny view inline at SurgSim/Framework/SceneElement.cpp (merge request Extend yaml loading)

246	<< className << ">" << " but this is a <" << getClassName() << ">.";
	Is this caught by the try/catch block in tryLoadNode?
247

on 2015-02-13 19:33 By Harald Scheirich view inline at SurgSim/Framework/SceneElement.cpp (merge request Extend yaml loading)

<< className << ">" << " but this is a <" << getClassName() << ">.";

@pnovotny no, while I was editing the yaml file there is a valid yaml that when the indentation is incorrect produces just an empty node after the classname the node with the label Name: for example will become the next in the list somehow, this should catch that

on 2015-02-12 23:43 By pnovotny view inline at SurgSim/Framework/SceneElement.cpp (merge request Extend yaml loading)

SURGSIM_ASSERT(className == getClassName()) << "Type in node does not match class, wanted <"

This line ends in whitespace, cpplint warning.

<< className << ">" << " but this is a <" << getClassName() << ">.";

on 2015-02-12 23:44 By pnovotny

Looks good, had some minor questions, cleanup suggestions. There is a cpplint warning I pointed out.

show votes

Version: 3

by Harald Scheirich on 2015-01-30 01:07 (over 9 years ago)

+1 -0

From: OpenSurgSim:feature/add-scene(9f14653d1f)
To: OpenSurgSim:master (64d626d61e)

Discussion 3
Commits -
Files Affected -
Tickets Affected -
Followers -

Show code comments

on 2015-01-30 02:43 By pnovotny

@hscheirich I know we have talked through this alot, but I thought I would flesh out a complete picture of an alternative, so others could comment. It follows our current use of Assets, and would make OSS a bit more consistent throughout. For example the Font is an Asset and can be loaded like:

  auto font = std::make_shared<Graphics::Font>();
  font->load("SomeFontFile.ext");

or there are convenience functions on things that use assets, like:

  auto textRep = std::make_shared<Graphics::OsgTextRepresentation>("Text");
  textRep->loadFont("SomeFontFile.ext")

So, to "load" something, we make it, then call load on it. And things that use Assets have convenience functions that do that for you, ie loadMesh, loadFont, etc. I think as we discussed, moving the loading to the actualy class follows this better. Could we have:

  auto scene = std::make_shared<Framework::Scene>() // this could be runtime->getScene, I don't care that much
  scene->load("Scene.yaml"); // this is the same as runtime->loadScene("Scene.yaml");

Then, I think the Scene should also have the following interface:

  scene->addSceneElements(std::string filename); // same as your new runtime->addSceneElements
  scene->addSceneElements(SceneElements elements);  //overloaded with new SceneElements class

These nicely go along with the functions already there:

  scene->addSceneElement(SceneElement element)
  scene->getSceneElements();
  Scene->getSceneElement(std::string filename);

Then similarly, we could add a "load" to BasicSceneElement.

  auto element = std::make_shared<Framework::BasicSceneElement>("element");
  element->load("element.yaml");

I know you object to doing this because it would introduce a new yaml format. But could this format be a list of SceneElements of lenght one. Instead of just a SceneElement, that would keep the indentation and formating the same between the serialization of elements and an element.

For completeness we could add a new SceneElements class, that is just derived from std::vector<SceneElement> and maybe Asset, so you could do this:

  auto elements = std::make_shared<SceneElements>();
  elements->load("multiple_elements.yaml");

To "clone" things, you use SceneElement::load and SceneElements::load and then add those obects to the scene. To "add" to the scene, you use file version of Scene::addSceneElements.

on 2015-01-30 02:57 By wdturner

There seem to be some warnings on all platforms and some errors on the Linux platforms. I think they are related to Doxygen, but you should take a look. Checkk out https://open.cdash.org/index.php?project=OpenSurgSim&date=

on 2015-01-30 23:08 By ryanbeasley

While I think it's odd for these functions to be on Runtime instead of Scene, I don't think it's enough of an issue to put effort into it.

show votes

Version: 2

by Harald Scheirich on 2015-01-23 19:30 (over 9 years ago)

19|

+1 -0

From: OpenSurgSim:feature/add-scene(2b037f636e)
To: OpenSurgSim:master (239c8aa868)

Discussion 19
Commits -
Files Affected -
Tickets Affected -
Followers -

Show code comments

on 2015-01-24 01:09 By jlenoir view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

344	if (tryLoadNode(fileName, &node))
	@hscheirich If i understand right, you are trying to load 1 node. Why is the method name plural ?
345	{

on 2015-01-26 19:10 By Harald Scheirich view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

344	if (tryLoadNode(fileName, &node))
	@jlenoir Either is fine, it is still a collection of nodes, i'll rename
345	{

on 2015-01-27 23:07 By jlenoir view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

if (tryLoadNode(fileName, &node))

@hscheirich I did not realize a YAML::Node is a collection of nodes/stuff...
But the class name (Node) being singular, I think we should stick to a singular method name for any methods manipulating such object.

on 2015-01-24 01:14 By jlenoir view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

bool Runtime::addSceneElements(const std::string& fileName)

@hscheirich Why is the method called addSceneElements and not addScene. The filename describe a scene anyway (a collection of scene element anyway, no ?).

on 2015-01-26 19:14 By Harald Scheirich view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

356	bool Runtime::addSceneElements(const std::string& fileName)
	@jlenoir I will write a longer explanation in the discussion
357	{

on 2015-01-24 01:16 By jlenoir view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

if (tryLoadNode(fileName, &node))

@hscheirich Homogenize formatting between this method and the previous one. This one has a space before the if, the one before does not. This method has no space before the result = true; while the one before has a space...

on 2015-01-24 01:31 By jlenoir view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

std::vector<std::shared_ptr<SceneElement>> Runtime::duplicateSceneElements(const std::string& fileName)

@hscheirich For me this is the wrong usage of a method "clone".
Usually the clone method is on the class/intance you want to clone, so I would expect it to be in SceneElement.
If I understand properly what is going on, you are not cloning any instance here, you are loading SceneElements from a file.
This seems to be like this method should be called getSceneElements or retrieveSceneElements.

on 2015-01-26 19:15 By Harald Scheirich view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

381	std::vector<std::shared_ptr<SceneElement>> Runtime::duplicateSceneElements(const std::string& fileName)
	@jlenoir I will write a longer explanation in the discussion
382	{

on 2015-01-24 01:34 By jlenoir view inline at SurgSim/Framework/Runtime.h (merge request Extend yaml loading)

29	}
	Usually our closing brace for namespace is followed by a semi-column.
30

on 2015-01-24 01:35 By jlenoir view inline at SurgSim/Framework/Runtime.h (merge request Extend yaml loading)

/// \return true if the loading succeeded and the scene was found, the loaded scene elements will have been added

return description is not precise enough. Loading succeeded and scene elements were adding successfully into the scene.

///         to the scene.

on 2015-01-24 01:38 By jlenoir view inline at SurgSim/Testing/TestingMain.cpp (merge request Extend yaml loading)

Commented out code should be cleaned up before merging.

on 2015-01-26 19:17 By Harald Scheirich view inline at SurgSim/Testing/TestingMain.cpp (merge request Extend yaml loading)

@jlenoir Thanks, this is the second time I forgot to change this !

on 2015-01-24 01:47 By jlenoir

@hscheirich I mostly have questions about method namings...see comments.
Once this is cleared, I will vote +1.

on 2015-01-26 19:32 By Harald Scheirich

@jlenoir Regarding the functionality/naming ... There are three different functionalities implemented

- loading
- composition
- duplication

Loading of a scene is done via the loadScene call, this should load and initialize a 'Scene' which while currently only consisting of a collection of SceneElements is still a construct in it's own right. I think we are eventually likely to add other parameters to the scene (e.g. The managers that implement this scene).

Composition, is the extension of a scene with more sceneElements (addSceneElements()), these are loaded in a way so that they can maintain references with the scene, and enable us for example to implement scenarios that rely on a base scene but extend that scene with different elements. Therefore this just loads a list of sceneelements, rather than a Scene. If we extended the scene (with managers for example) we would have to ignore the extra information in the 'added' scene, this is why I decided to downgrade the data (and use a different name for loading.

Duplication, I see a yaml file as a serialized Object, i.e. it is equivalent to the object, no in this case we don't pass a SceneElement in but the name of the yaml file, the functionality is still to duplicate/clone the contained instances rather than just load them. I also think that in the long run we will shortcut the yaml file and clone from the already loaded node. Each of these functionalities are easier to achieve that a clone operation on the object level (at least at this stage). We are not just retrieving at this point (addSceneElements) is a plain retrieval, because we are retrieving in a way that lets us retrieve multiple copies of the same thing.

Additionally I chose a list of scene elements rather than just a scene element for the composition and duplication operations because I was assuming that in most cases one scene element was not going to be enough, we also do not have a hierarchical scene element model, therefore the list.

I hope this explains the reason for the naming better, is there a way I can add documentation to make this clearer or is there a better way to name things with these separate operations in mind ?

on 2015-01-27 00:44 By haizhouW view inline at SurgSim/Framework/Runtime.h (merge request Extend yaml loading)

///         to the scene.

We have a method bool addSceneElement(std::shared_ptr<SceneElement> sceneElement); in Runtime

This method here may cause confusion (although it has the s in the end of its name).


Also, I'm thinking, why not move the add/remove sceneElement(s) features into Scene?
After all, Runtime deals with Scene and Scene deals with SceneElement.

bool addSceneElements(const std::string& fileName);

on 2015-01-27 23:00 By jlenoir

@hscheirich I agree duplication and composition should return a list of SceneElement, this is fine by me.
I also agree that a yaml file could potentially in the future have more than SceneElements (Managers,....).

If I understand properly what you said, the object-based duplication process (clone method on the SceneElement itself) is our goal but cannot be achieve easily right now. So we implement this functionality via Runtime. What needs to happen before we get there ? You were talking about static registration, is that the issue ?
I am not pushing for this to happen now, just trying to get a grasp of the all context.

For the clone method naming, I would prefer duplicateSceneElements I think.
In most language clone and duplicate both create a copy of an object, but the clone is the exact replica (even object uuid is equal for example, creating the exact same object), while the duplicate will replicate the content but identify the newly created copy as a separate object (different object uuid).

on 2015-01-29 02:46 By Harald Scheirich

@jlenoir I'll rename the clone function, I don't know if we really need an object base clone, while access through the runtime is not ideal the way of doing it (using the node structure as a stored version of the object) is sufficient for our needs. Right now I see now need to start the effort of implementing a copy operation on all our components

on 2015-01-29 04:02 By pnovotny

@hscheirich Could we do a simple SceneElement::load(filename), that wraps this yaml node stuff. Would this still need the lock?

on 2015-01-29 05:07 By Harald Scheirich

@pnovotny yes it would need the lock.

Another issue would be the file format, one sceneElement looks like this

SurgSim::Framework::BasicSceneElement:
  Name: THeName
  Components: 
     - ...

A list of SceneElements look like this

- SurgSim::Framework::BasicSceneElement:
  Name: THeName
  Components: 
     - ...
- SurgSim::Framework::..

note the '-' as far as I know you would not be able to load one sceneelement as a list or vice versa, but the files wouldn't tell you that from the name unless we are very strict about naming, creating the issue that you would have to look at the file itself to detect how to load it. I.e. via addElements/duplicateElements or via SceneElement::load(). I.e. I would probably have loadSceneElement(), implemented as a loadSceneElements(), just to keep them compatible.

We could just rename clone/duplicateScemeElements into loadSceneElements() would that be sufficient ?

show votes

Version: 1

by Harald Scheirich on 2015-01-21 23:09 (over 9 years ago)

+1 -0

From: OpenSurgSim:feature/add-scene(7ab53e6307)
To: OpenSurgSim:master (239c8aa868)

Discussion 9
Commits -
Files Affected -
Tickets Affected -
Followers -

Show code comments

on 2015-01-22 03:16 By haizhouW view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

366	{
	I think line 366 ~ 369 could be replaced by `m_scene->addSceneElements(element);`
367	m_scene->addSceneElements(elements);

on 2015-01-22 21:54 By Harald Scheirich view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

366	{
	@haizhouW ah, yes, that is why it put it in ;)
367	m_scene->addSceneElements(elements);

on 2015-01-22 03:33 By haizhouW view inline at SurgSim/Framework/UnitTests/RuntimeTest.cpp (merge request Extend yaml loading)

runtime->step();

What if a user called runtime->addSceneElements("SceneTestData/scene.yaml");?
(Also, the around way around, when runtime->loadScene("SceneTestData/elements.yaml");)

Should we give some warning?

boost::this_thread::sleep(boost::posix_time::milliseconds(150));

on 2015-01-22 21:58 By Harald Scheirich view inline at SurgSim/Framework/UnitTests/RuntimeTest.cpp (merge request Extend yaml loading)

150	runtime->step();
	@haizhouW Yes we should
151	boost::this_thread::sleep(boost::posix_time::milliseconds(150));

on 2015-01-22 21:17 By ryanbeasley view inline at SurgSim/Framework/Runtime.cpp (merge request Extend yaml loading)

404	bool Runtime::tryLoadNodes(const std::string& fileName, YAML::Node* nodes)
	Why do this instead of `*nodes = YAML::LoadFile(path);`
405	{

on 2015-01-22 21:35 By ryanbeasley view inline at SurgSim/Framework/SceneElement.cpp (merge request Extend yaml loading)

253	if (data["Name"].IsScalar())
	I'm curious why this if-test was added.
254	{

on 2015-01-22 21:57 By Harald Scheirich view inline at SurgSim/Framework/SceneElement.cpp (merge request Extend yaml loading)

if (data["Name"].IsScalar())

@ryanbeasley I'll add a warning if its not found, this becomes more an issue when hand writing yaml files, it is easy to forget parts, before if the name was not there the old line 250 was going to cause an exception, name not existing can also be a side effect of the yaml formatting being wrong as the structure will shift

on 2015-01-22 22:06 By ryanbeasley view inline at SurgSim/Framework/SceneElement.cpp (merge request Extend yaml loading)

253	if (data["Name"].IsScalar())
	@hscheirich Oh, I see. Makes sense.
254	{

on 2015-01-22 21:36 By ryanbeasley view inline at SurgSim/Testing/TestingMain.cpp (merge request Extend yaml loading)

26	//loggerManager->setDefaultOutput(std::make_shared<SurgSim::Framework::NullOutput>());
	Looks like a debugging change that slipped in.
27

show votes

Extend yaml loading

Version: 6

Votes

Followers

Version: 5

Votes

Version: 4

Votes

Version: 3

Votes

Version: 2

Votes

Version: 1

Votes