[amberscript] Add CLEAR_COLOR parsing. (google#416)

This Cl adds CLEAR_COLOR parsing into the AmberScript parser. The EXPECT EQ was also fixed to use the correct 0-255 range instead of 0.0-1.0 range for colour expectations. Fixes google#344.
petermcneeleychromium · Mar 28, 2019 · 367b63b · 367b63b
1 parent d68c489
commit 367b63b
Show file tree

Hide file tree

Showing 8 changed files with 274 additions and 26 deletions.
diff --git a/docs/amber_script.md b/docs/amber_script.md
@@ -303,8 +303,6 @@ RUN <pipeline_name> DRAW_ARRAY INDEXED AS <topology> \
 ```
 # Sets the clear color to use for |pipeline| which must be a `graphics`
 # pipeline. The colors are integers from 0 - 255.
-# TODO(dsinclair): Do we need to allow different types here to handle different
-#                  buffer formats?
 CLEAR_COLOR <pipeline> <r (0 - 255)> <g (0 - 255)> <b (0 - 255)> <a (0 - 255)>
 
 # Instructs the |pipeline| which must be a `graphics` pipeline to execute the

diff --git a/src/amberscript/parser.cc b/src/amberscript/parser.cc
@@ -76,6 +76,8 @@ Result Parser::Parse(const std::string& data) {
       r = ParseBuffer();
     } else if (tok == "CLEAR") {
       r = ParseClear();
+    } else if (tok == "CLEAR_COLOR") {
+      r = ParseClearColor();
     } else if (tok == "COPY") {
       r = ParseCopy();
     } else if (tok == "EXPECT") {
@@ -903,7 +905,7 @@ Result Parser::ParseRun() {
 
   if (token->IsInteger()) {
     if (!pipeline->IsCompute())
-      return Result("RUN command requires compute pipeline, got graphics");
+      return Result("RUN command requires compute pipeline");
 
     auto cmd = MakeUnique<ComputeCommand>(pipeline);
     cmd->SetX(token->AsUint32());
@@ -930,7 +932,7 @@ Result Parser::ParseRun() {
 
   if (token->AsString() == "DRAW_RECT") {
     if (!pipeline->IsGraphics())
-      return Result("RUN command requires graphics pipeline, got compute");
+      return Result("RUN command requires graphics pipeline");
 
     token = tokenizer_->NextToken();
     if (token->IsEOS() || token->IsEOL())
@@ -992,7 +994,7 @@ Result Parser::ParseRun() {
 
   if (token->AsString() == "DRAW_ARRAY") {
     if (!pipeline->IsGraphics())
-      return Result("RUN command requires graphics pipeline, got compute");
+      return Result("RUN command requires graphics pipeline");
 
     auto cmd = MakeUnique<DrawArraysCommand>(pipeline, PipelineData{});
 
@@ -1013,7 +1015,7 @@ Result Parser::ParseClear() {
   if (!pipeline)
     return Result("unknown pipeline for CLEAR command: " + token->AsString());
   if (!pipeline->IsGraphics())
-    return Result("CLEAR command requires graphics pipeline, got compute");
+    return Result("CLEAR command requires graphics pipeline");
 
   auto cmd = MakeUnique<ClearCommand>(pipeline);
   script_->AddCommand(std::move(cmd));
@@ -1127,26 +1129,26 @@ Result Parser::ParseExpect() {
     if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255)
       return Result("invalid R value in EXPECT command");
     token->ConvertToDouble();
-    probe->SetR(token->AsFloat());
+    probe->SetR(token->AsFloat() / 255.f);
 
     token = tokenizer_->NextToken();
     if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255)
       return Result("invalid G value in EXPECT command");
     token->ConvertToDouble();
-    probe->SetG(token->AsFloat());
+    probe->SetG(token->AsFloat() / 255.f);
 
     token = tokenizer_->NextToken();
     if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255)
       return Result("invalid B value in EXPECT command");
     token->ConvertToDouble();
-    probe->SetB(token->AsFloat());
+    probe->SetB(token->AsFloat() / 255.f);
 
     if (probe->IsRGBA()) {
       token = tokenizer_->NextToken();
       if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255)
         return Result("invalid A value in EXPECT command");
       token->ConvertToDouble();
-      probe->SetA(token->AsFloat());
+      probe->SetA(token->AsFloat() / 255.f);
     }
 
     script_->AddCommand(std::move(probe));
@@ -1235,5 +1237,65 @@ Result Parser::ParseCopy() {
   return ValidateEndOfStatement("COPY command");
 }
 
+Result Parser::ParseClearColor() {
+  auto token = tokenizer_->NextToken();
+  if (!token->IsString())
+    return Result("missing pipeline name for CLEAR_COLOR command");
+
+  auto* pipeline = script_->GetPipeline(token->AsString());
+  if (!pipeline) {
+    return Result("unknown pipeline for CLEAR_COLOR command: " +
+                  token->AsString());
+  }
+  if (!pipeline->IsGraphics()) {
+    return Result("CLEAR_COLOR command requires graphics pipeline");
+  }
+
+  auto cmd = MakeUnique<ClearColorCommand>(pipeline);
+
+  token = tokenizer_->NextToken();
+  if (token->IsEOL() || token->IsEOS())
+    return Result("missing R value for CLEAR_COLOR command");
+  if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255) {
+    return Result("invalid R value for CLEAR_COLOR command: " +
+                  token->ToOriginalString());
+  }
+  token->ConvertToDouble();
+  cmd->SetR(token->AsFloat() / 255.f);
+
+  token = tokenizer_->NextToken();
+  if (token->IsEOL() || token->IsEOS())
+    return Result("missing G value for CLEAR_COLOR command");
+  if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255) {
+    return Result("invalid G value for CLEAR_COLOR command: " +
+                  token->ToOriginalString());
+  }
+  token->ConvertToDouble();
+  cmd->SetG(token->AsFloat() / 255.f);
+
+  token = tokenizer_->NextToken();
+  if (token->IsEOL() || token->IsEOS())
+    return Result("missing B value for CLEAR_COLOR command");
+  if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255) {
+    return Result("invalid B value for CLEAR_COLOR command: " +
+                  token->ToOriginalString());
+  }
+  token->ConvertToDouble();
+  cmd->SetB(token->AsFloat() / 255.f);
+
+  token = tokenizer_->NextToken();
+  if (token->IsEOL() || token->IsEOS())
+    return Result("missing A value for CLEAR_COLOR command");
+  if (!token->IsInteger() || token->AsInt32() < 0 || token->AsInt32() > 255) {
+    return Result("invalid A value for CLEAR_COLOR command: " +
+                  token->ToOriginalString());
+  }
+  token->ConvertToDouble();
+  cmd->SetA(token->AsFloat() / 255.f);
+
+  script_->AddCommand(std::move(cmd));
+  return ValidateEndOfStatement("CLEAR_COLOR command");
+}
+
 }  // namespace amberscript
 }  // namespace amber
diff --git a/src/amberscript/parser.h b/src/amberscript/parser.h
@@ -63,6 +63,7 @@ class Parser : public amber::Parser {
   Result ParsePipelineIndexData(Pipeline*);
   Result ParseRun();
   Result ParseClear();
+  Result ParseClearColor();
   Result ParseExpect();
   Result ParseCopy();
 

diff --git a/src/amberscript/parser_test.cc b/src/amberscript/parser_test.cc
@@ -2988,8 +2988,7 @@ RUN my_pipeline 2 4 5)";
   Parser parser;
   Result r = parser.Parse(in);
   ASSERT_FALSE(r.IsSuccess());
-  ASSERT_EQ("12: RUN command requires compute pipeline, got graphics",
-            r.Error());
+  ASSERT_EQ("12: RUN command requires compute pipeline", r.Error());
 }
 
 TEST_F(AmberScriptParserTest, RunComputeMissingParams) {
@@ -3141,8 +3140,7 @@ RUN my_pipeline DRAW_RECT POS 2 4 SIZE 10 20)";
   Parser parser;
   Result r = parser.Parse(in);
   ASSERT_FALSE(r.IsSuccess());
-  ASSERT_EQ("12: RUN command requires graphics pipeline, got compute",
-            r.Error());
+  ASSERT_EQ("12: RUN command requires graphics pipeline", r.Error());
 }
 
 TEST_F(AmberScriptParserTest, RunDrawRectWithMissingPipeline) {
@@ -3466,8 +3464,7 @@ CLEAR my_pipeline)";
   Parser parser;
   Result r = parser.Parse(in);
   ASSERT_FALSE(r.IsSuccess());
-  ASSERT_EQ("12: CLEAR command requires graphics pipeline, got compute",
-            r.Error());
+  ASSERT_EQ("12: CLEAR command requires graphics pipeline", r.Error());
 }
 
 TEST_F(AmberScriptParserTest, ClearExtraParams) {
@@ -3527,9 +3524,9 @@ EXPECT my_fb IDX 5 6 SIZE 250 150 EQ_RGB 2 128 255)";
   EXPECT_EQ(6U, probe->GetY());
   EXPECT_EQ(250U, probe->GetWidth());
   EXPECT_EQ(150U, probe->GetHeight());
-  EXPECT_EQ(2U, probe->GetR());
-  EXPECT_EQ(128U, probe->GetG());
-  EXPECT_EQ(255U, probe->GetB());
+  EXPECT_FLOAT_EQ(2.f / 255.f, probe->GetR());
+  EXPECT_FLOAT_EQ(128.f / 255.f, probe->GetG());
+  EXPECT_FLOAT_EQ(255.f / 255.f, probe->GetB());
 }
 
 TEST_F(AmberScriptParserTest, ExpectRGBA) {
@@ -3569,10 +3566,10 @@ EXPECT my_fb IDX 2 7 SIZE 20 88 EQ_RGBA 2 128 255 99)";
   EXPECT_EQ(7U, probe->GetY());
   EXPECT_EQ(20U, probe->GetWidth());
   EXPECT_EQ(88U, probe->GetHeight());
-  EXPECT_EQ(2U, probe->GetR());
-  EXPECT_EQ(128U, probe->GetG());
-  EXPECT_EQ(255U, probe->GetB());
-  EXPECT_EQ(99U, probe->GetA());
+  EXPECT_FLOAT_EQ(2.f / 255.f, probe->GetR());
+  EXPECT_FLOAT_EQ(128.f / 255.f, probe->GetG());
+  EXPECT_FLOAT_EQ(255.f / 255.f, probe->GetB());
+  EXPECT_FLOAT_EQ(99.f / 255.f, probe->GetA());
 }
 
 TEST_F(AmberScriptParserTest, ExpectMissingBufferName) {
@@ -4257,5 +4254,152 @@ COPY from dest)";
   EXPECT_EQ("4: expected 'TO' after COPY and buffer name", r.Error());
 }
 
+TEST_F(AmberScriptParserTest, ClearColor) {
+  std::string in = R"(
+SHADER vertex my_shader PASSTHROUGH
+SHADER fragment my_fragment GLSL
+# GLSL Shader
+END
+BUFFER my_fb FORMAT R32G32B32A32_SFLOAT
+
+PIPELINE graphics my_pipeline
+  ATTACH my_shader
+  ATTACH my_fragment
+END
+
+CLEAR_COLOR my_pipeline 255 128 64 32)";
+
+  Parser parser;
+  Result r = parser.Parse(in);
+  ASSERT_TRUE(r.IsSuccess()) << r.Error();
+
+  auto script = parser.GetScript();
+  const auto& commands = script->GetCommands();
+  ASSERT_EQ(1U, commands.size());
+
+  auto* cmd = commands[0].get();
+  ASSERT_TRUE(cmd->IsClearColor());
+
+  auto* clr = cmd->AsClearColor();
+  EXPECT_FLOAT_EQ(255.f / 255.f, clr->GetR());
+  EXPECT_FLOAT_EQ(128.f / 255.f, clr->GetG());
+  EXPECT_FLOAT_EQ(64.f / 255.f, clr->GetB());
+  EXPECT_FLOAT_EQ(32.f / 255.f, clr->GetA());
+}
+
+TEST_F(AmberScriptParserTest, ClearColorWithComputePipeline) {
+  std::string in = R"(
+SHADER compute my_shader GLSL
+# shader
+END
+
+PIPELINE compute my_pipeline
+  ATTACH my_shader
+END
+
+CLEAR_COLOR my_pipeline 255 128 64 32)";
+
+  Parser parser;
+  Result r = parser.Parse(in);
+  ASSERT_FALSE(r.IsSuccess());
+  EXPECT_EQ("10: CLEAR_COLOR command requires graphics pipeline", r.Error());
+}
+
+TEST_F(AmberScriptParserTest, ClearColorMissingPipeline) {
+  std::string in = "CLEAR_COLOR 255 255 255 255";
+
+  Parser parser;
+  Result r = parser.Parse(in);
+  ASSERT_FALSE(r.IsSuccess());
+  EXPECT_EQ("1: missing pipeline name for CLEAR_COLOR command", r.Error());
+}
+
+TEST_F(AmberScriptParserTest, ClearColorInvalidPipeline) {
+  std::string in = "CLEAR_COLOR unknown_pipeline 255 255 255 255";
+
+  Parser parser;
+  Result r = parser.Parse(in);
+  ASSERT_FALSE(r.IsSuccess());
+
+  EXPECT_EQ("1: unknown pipeline for CLEAR_COLOR command: unknown_pipeline",
+            r.Error());
+}
+
+struct ClearColorTestData {
+  std::string data;
+  std::string error;
+};
+using AmberScriptParserClearColorTest =
+    testing::TestWithParam<ClearColorTestData>;
+TEST_P(AmberScriptParserClearColorTest, InvalidParams) {
+  auto test_data = GetParam();
+
+  std::string in = R"(
+SHADER vertex my_shader PASSTHROUGH
+SHADER fragment my_fragment GLSL
+# GLSL Shader
+END
+BUFFER my_fb FORMAT R32G32B32A32_SFLOAT
+
+PIPELINE graphics my_pipeline
+  ATTACH my_shader
+  ATTACH my_fragment
+END
+
+CLEAR_COLOR my_pipeline )" +
+                   test_data.data;
+
+  Parser parser;
+  Result r = parser.Parse(in);
+  ASSERT_FALSE(r.IsSuccess()) << test_data.data;
+  EXPECT_EQ(std::string("13: ") + test_data.error, r.Error()) << test_data.data;
+}
+
+INSTANTIATE_TEST_CASE_P(
+    AmberScriptParserClearColorTests,
+    AmberScriptParserClearColorTest,
+    testing::Values(
+        ClearColorTestData{"", "missing R value for CLEAR_COLOR command"},
+        ClearColorTestData{"255", "missing G value for CLEAR_COLOR command"},
+        ClearColorTestData{"255 255",
+                           "missing B value for CLEAR_COLOR command"},
+        ClearColorTestData{"255 255 255",
+                           "missing A value for CLEAR_COLOR command"},
+        ClearColorTestData{"INVALID 255 255 255",
+                           "invalid R value for CLEAR_COLOR command: INVALID"},
+        ClearColorTestData{"255 INVALID 255 255",
+                           "invalid G value for CLEAR_COLOR command: INVALID"},
+        ClearColorTestData{"255 255 INVALID 255",
+                           "invalid B value for CLEAR_COLOR command: INVALID"},
+        ClearColorTestData{"255 255 255 INVALID",
+                           "invalid A value for CLEAR_COLOR command: INVALID"},
+        ClearColorTestData{"255 255 255 255 EXTRA",
+                           "extra parameters after CLEAR_COLOR command"},
+        ClearColorTestData{"-1 255 255 255",
+                           "invalid R value for CLEAR_COLOR command: -1"},
+        ClearColorTestData{"5.2 255 255 255",
+                           "invalid R value for CLEAR_COLOR command: 5.2"},
+        ClearColorTestData{"256 255 255 255",
+                           "invalid R value for CLEAR_COLOR command: 256"},
+        ClearColorTestData{"255 -1 255 255",
+                           "invalid G value for CLEAR_COLOR command: -1"},
+        ClearColorTestData{"255 5.2 255 255",
+                           "invalid G value for CLEAR_COLOR command: 5.2"},
+        ClearColorTestData{"255 256 255 255",
+                           "invalid G value for CLEAR_COLOR command: 256"},
+        ClearColorTestData{"255 255 -1 255",
+                           "invalid B value for CLEAR_COLOR command: -1"},
+        ClearColorTestData{"255 255 5.2 255",
+                           "invalid B value for CLEAR_COLOR command: 5.2"},
+        ClearColorTestData{"255 255 256 255",
+                           "invalid B value for CLEAR_COLOR command: 256"},
+        ClearColorTestData{"255 255 255 -1",
+                           "invalid A value for CLEAR_COLOR command: -1"},
+        ClearColorTestData{"255 255 255 5.2",
+                           "invalid A value for CLEAR_COLOR command: 5.2"},
+        ClearColorTestData{"255 255 255 256",
+                           "invalid A value for CLEAR_COLOR "
+                           "command: 256"}), );  // NOLINT(whitespace/parens)
+
 }  // namespace amberscript
 }  // namespace amber
diff --git a/src/command.h b/src/command.h
@@ -274,6 +274,7 @@ class ProbeCommand : public Probe {
   void SetHeight(float h) { height_ = h; }
   float GetHeight() const { return height_; }
 
+  // Colours are stored in the range 0.0 - 1.0
   void SetR(float r) { r_ = r; }
   float GetR() const { return r_; }
 
@@ -418,6 +419,7 @@ class ClearColorCommand : public PipelineCommand {
   explicit ClearColorCommand(Pipeline* pipeline);
   ~ClearColorCommand() override;
 
+  // Colours are stored in the range 0.0 - 1.0
   void SetR(float r) { r_ = r; }
   float GetR() const { return r_; }