Element::Get ignores default input for elements

**Original report ([archived issue](https://osrf-migration.github.io/sdformat-gh-pages/#!/osrf/sdformat/issues/213)) by Louise Poubel (Bitbucket: [chapulina](https://bitbucket.org/%7B5cfa2075-477b-4ded-bdb9-8d2479544ec4%7D/), GitHub: [chapulina](https://github.com/chapulina)).**

----------------------------------------
# Prerequisites

* [ X ] Put an X between the brackets on this line if you have done all of the following:
* Checked the Q&A board for common solutions: http://answers.gazebosim.org
* Checked that your issue isn't already filed.
* Checked that there is not already an Ignition package that provides the described functionality: https://ignitionrobotics.org/libs

# Description

The `Element::Get` function which accepts an optional default value will always ignore that value in favour of the default value defined in the spec.

# Steps to Reproduce

I would expect the following tests to pass:

~~~
diff -r 2df1b5b5d05f3ff52a97eaca8f17c2cddb64705b src/Root_TEST.cc
--- a/src/Root_TEST.cc Thu Jan 31 20:37:41 2019 +0000
+++ b/src/Root_TEST.cc Tue Feb 26 18:42:00 2019 -0800
@@ -23,6 +23,7 @@
#include "sdf/Light.hh"
#include "sdf/Model.hh"
#include "sdf/Root.hh"
+#include "sdf/Sensor.hh"

/////////////////////////////////////////////////
TEST(DOMRoot, Construction)
@@ -63,6 +64,13 @@
" "
" "
" "
+ " "
+ " "
+ " box_col"
+ " "
+ " 1"
+ " 100"
+ " "
" "
" "
" "
@@ -95,6 +103,16 @@
EXPECT_NE(nullptr, collision->Element());
EXPECT_EQ("box_col", collision->Name());

+ const sdf::Sensor *sensor = link->SensorByIndex(0);
+ ASSERT_NE(nullptr, sensor);
+ EXPECT_EQ("sensor_contact", sensor->Name());
+ ASSERT_NE(nullptr, sensor->Element());
+ EXPECT_TRUE(sensor->Element()->HasElement("always_on"));
+ EXPECT_FALSE(sensor->Element()->HasElement("topic"));
+ auto [topic, success] = sensor->Element()->Get("topic", "default_value");
+ EXPECT_EQ("default_value", topic);
+ EXPECT_FALSE(success);
+
EXPECT_TRUE(root.LightNameExists("sun"));
EXPECT_EQ(1u, root.LightCount());
const sdf::Light *light = root.LightByIndex(0);

diff -r 2df1b5b5d05f3ff52a97eaca8f17c2cddb64705b src/SDF_TEST.cc
--- a/src/SDF_TEST.cc Thu Jan 31 20:37:41 2019 +0000
+++ b/src/SDF_TEST.cc Tue Feb 26 18:42:00 2019 -0800
@@ -173,6 +173,20 @@
EXPECT_EQ(elem->Get("name", "default_value").first, "model2");
EXPECT_TRUE(elem->Get("name", "default_value").second);

+ {
+ EXPECT_TRUE(elem->HasElement("static"));
+ auto [value, success] = elem->Get("static", true);
+ EXPECT_FALSE(value);
+ EXPECT_TRUE(success);
+ }
+
+ {
+ EXPECT_FALSE(elem->HasElement("self_collide"));
+ auto [value, success] = elem->Get("self_collide", true);
+ EXPECT_TRUE(value);
+ EXPECT_FALSE(success);
+ }
+
// Remove model2
elem->RemoveFromParent();
~~~

However, they fail because the function will always fallback to the default value from the spec, rather than using the passed value.

**Expected behavior:**

I'd expect the default value passed by the user to override the default from the spec, otherwise there's no point on calling this function.

**Actual behavior:**

The tests fail:

~~~
[ RUN ] DOMRoot.StringParse
/home/developer/sdformat/src/Root_TEST.cc:113: Failure
Expected equality of these values:
"default_value"
topic
Which is: "__default__"
/home/developer/sdformat/src/Root_TEST.cc:114: Failure
Value of: success
Actual: true
Expected: false
[ FAILED ] DOMRoot.StringParse (37 ms)
~~~

~~~
[ RUN ] SDF.ElementRemoveFromParent
Warning [parser.cc:528] Converting a deprecated SDF source[data-string].
/home/developer/sdformat/src/SDF_TEST.cc:186: Failure
Value of: value
Actual: false
Expected: true
/home/developer/sdformat/src/SDF_TEST.cc:187: Failure
Value of: success
Actual: true
Expected: false
[ FAILED ] SDF.ElementRemoveFromParent (35 ms)
~~~

**Reproduces how often:**

Always

# Versions

sdf8

# Additional Information

One way to solve this issue is to remove the line that uses the default value from the spec:

~~~
diff -r 2df1b5b5d05f3ff52a97eaca8f17c2cddb64705b include/sdf/Element.hh
--- a/include/sdf/Element.hh Thu Jan 31 20:37:41 2019 +0000
+++ b/include/sdf/Element.hh Tue Feb 26 18:42:00 2019 -0800
@@ -470,10 +470,6 @@
{
result.first = this->GetElementImpl(_key)->Get();
}
- else if (this->HasElementDescription(_key))
- {
- result.first = this->GetElementDescription(_key)->Get();
- }
else
{
result.second = false;
~~~

However, that causes tests which use the `Get` version that doesn't accept a default value to fail, like:

~~~
/home/developer/sdformat/test/integration/fixed_joint_reduction.cc:334: Failure
Expected equality of these values:
urdf_child_link_2_col->Get("max_contacts")
Which is: 0
10
~~~

That's because that function is calling the other one, passing a default value:

~~~
///////////////////////////////////////////////
template
T Element::Get(const std::string &_key) const
{
T result = T();

std::pair ret = this->Get(_key, result);

return ret.first;
}
~~~

I first wanted to check if this is the expected behaviour. If it isn't, I can make a PR fixing all cases.

Start with Element::Get in include/sdf/Element.hh, then run the cases shown in src/Root_TEST.cc and src/SDF_TEST.cc. Check the no-default behavior in integration/fixed_joint_reduction.cc as well. Done means an explicitly supplied default is respected while the overload without a default still uses spec defaults, with the affected tests passing.

Written by the indexing model from the issue text.