Skip to content

Commit 3b24b4e

Browse files
cdtwiggmeta-codesync[bot]
authored andcommitted
Rework how we handle sections in the parameter transform.
Summary: After D86041361 landed, we suddenly started getting a ton of warnings of the format: [MOMENTUM][WARNING]: Invalid line under [ParameterSets] section; ignoring limit b_l_pinky3_rz minmax [-0.1, 1.57] This was because we suddenly started flagging when we saw something in the parameter sets section that didn't look like a parameterset. This is IMO a good change to make, but it causes problems with the current parser. It basically just runs all the parsers (parameter sets, parameter transform, parameter limits) over the entire file, basically ignoring the sections. Thus, when the parametersets parser sees a line that looks like a parameter limit, it issues a warning. One fix would be to just back out that change. However, I do think the current approach is not ideal, because it means we have to constantly update all the parsers to ignore stuff from the other parsers, which is potentially error-prone. Instead, I'm suggesting going ahead and breaking the document up into sections in a pre-process. Then for each section we can parse just that section. This turned out to be a little trickier than I expected, however, because in D86041361 we also added the ability to have the same section appear more than once in the file. We could just glue them together, but this screws up line numbers (used for better error messages). Therefore I'm introducing a new glass that appends all the sections _implicitly_, keeping tracking of the start/end line numbers for each. Reviewed By: jeongseok-meta, cstollmeta Differential Revision: D86348623
1 parent 4e349fe commit 3b24b4e

File tree

8 files changed

+368
-76
lines changed

8 files changed

+368
-76
lines changed

cmake/build_variables.bzl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,7 @@ io_skeleton_public_headers = [
384384
"io/skeleton/parameter_limits_io.h",
385385
"io/skeleton/parameter_transform_io.h",
386386
"io/skeleton/parameters_io.h",
387+
"io/skeleton/utility.h",
387388
]
388389

389390
io_skeleton_sources = [
@@ -392,6 +393,7 @@ io_skeleton_sources = [
392393
"io/skeleton/parameter_limits_io.cpp",
393394
"io/skeleton/parameter_transform_io.cpp",
394395
"io/skeleton/parameters_io.cpp",
396+
"io/skeleton/utility.cpp",
395397
]
396398

397399
io_skeleton_test_sources = [

momentum/io/skeleton/parameter_limits_io.cpp

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
#include "momentum/character/parameter_transform.h"
1313
#include "momentum/character/skeleton.h"
1414
#include "momentum/common/log.h"
15+
#include "momentum/common/string.h"
16+
#include "momentum/io/skeleton/utility.h"
1517
#include "momentum/math/constants.h"
1618
#include "momentum/math/utility.h"
1719

@@ -609,23 +611,30 @@ void parseEllipsoid(const std::string& jointName, ParameterLimits& pl, Tokenizer
609611

610612
} // namespace
611613

614+
// Internal overload that accepts SectionContent
612615
ParameterLimits parseParameterLimits(
613-
const std::string& data,
616+
const io_detail::SectionContent& content,
614617
const Skeleton& skeleton,
615618
const ParameterTransform& parameterTransform) {
616619
ParameterLimits pl;
617620

618-
std::istringstream f(data);
621+
auto iterator = content.begin();
619622
std::string line;
620-
size_t lineIndex = 0;
621-
while (std::getline(f, line)) {
622-
++lineIndex;
623+
while (iterator.getline(line)) {
624+
const size_t lineIndex = iterator.currentLine();
623625

624626
// erase all comments
625627
line = line.substr(0, line.find_first_of('#'));
626628

629+
// Skip empty lines
630+
if (trim(line).empty()) {
631+
continue;
632+
}
633+
627634
// ignore everything but limits
628635
if (line.find("limit") != 0) {
636+
MT_LOGW(
637+
"Ignoring invalid line under [ParameterLimits] section at line {}: {}", lineIndex, line);
629638
continue;
630639
}
631640

@@ -672,6 +681,19 @@ ParameterLimits parseParameterLimits(
672681
return pl;
673682
}
674683

684+
// Public API wrapper for backward compatibility
685+
ParameterLimits parseParameterLimits(
686+
const std::string& data,
687+
const Skeleton& skeleton,
688+
const ParameterTransform& parameterTransform,
689+
size_t lineOffset) {
690+
io_detail::SectionContent content;
691+
if (!data.empty()) {
692+
content.addSegment(data, lineOffset);
693+
}
694+
return parseParameterLimits(content, skeleton, parameterTransform);
695+
}
696+
675697
namespace {
676698

677699
std::string vecToString(Eigen::Ref<const Eigen::VectorXf> vec) {

momentum/io/skeleton/parameter_limits_io.h

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,24 @@
1212

1313
namespace momentum {
1414

15+
namespace io_detail {
16+
// Forward declaration
17+
class SectionContent;
18+
} // namespace io_detail
19+
20+
// Internal overload for use within momentum parsing
1521
ParameterLimits parseParameterLimits(
16-
const std::string& data,
22+
const io_detail::SectionContent& content,
1723
const Skeleton& skeleton,
1824
const ParameterTransform& parameterTransform);
1925

26+
// Public API for external use
27+
ParameterLimits parseParameterLimits(
28+
const std::string& data,
29+
const Skeleton& skeleton,
30+
const ParameterTransform& parameterTransform,
31+
size_t lineOffset = 0);
32+
2033
std::string writeParameterLimits(
2134
const ParameterLimits& parameterLimits,
2235
const Skeleton& skeleton,

0 commit comments

Comments
 (0)