Introduce new cli-tool by j-beyer · Pull Request #24 · cginternals/glkernel

j-beyer · 2018-02-12T13:49:19Z

This PR introduces the new command line interface for glkernel.

As discussed in our e-mail, there are still open issues which can be found under https://github.com/p-otto/glkernel/issues

#25 should be merged before this, to make the code generation work correctly.
Additionally, #26 should be merged together with this, as it removes the old command line interface.

Rewrite CLI using cppassist::CommandLineProgram

…i-tool # Conflicts: # source/tools/glkernel-cli/main.cpp

…to access kernel

#22 cli api changes

…ascript, and kernel export to JSON

… given

Add --force switch

Working: - Creating kernels from JS (currently only kernel<float>) - Calling e.g. kernel.sequence.uniform() Missing: - Expose more functions

- Added scale::range and sort::distance - Figured out JS default parameters (see sort.distance) - Started JS-sided detection of parameter type

… to array

Scripting

…rn reference at end of file

…ing implementation to cpp file

Cleanup helper.h

Return kernel object in every js function

scheibel · 2018-02-22T17:24:39Z

source/tools/glkernel-cmd/KernelGeneration.cpp


 namespace
 {
+    using uint = unsigned int;


It seems like this type isn't used anymore. Additionally, I advise against using this type.

This was a compile fix for the to-be-deprecated glkernel-cmd.

scheibel · 2018-02-22T17:26:41Z

source/tools/glkernel-cli/main.cpp

+}
+
+std::string extractOutputFormat(const std::string & outFileName, const bool shouldConvert) {
+    if (outFileName.find('.') == std::string::npos)


There is a doubled search for '.' in the string. Search it once (outFileName.find_last_of('.')), store the result and reuse it both times.

scheibel · 2018-02-22T17:27:01Z

source/tools/glkernel-cli/main.cpp

+
+
+std::string extractInputFormat(const std::string & inFileName) {
+    if (inFileName.find('.') == std::string::npos)


There is a doubled search for '.' in the string. Search it once (inFileName.find_last_of('.')), store the result and reuse it both times.

Generally, you're performing file name operations, that are implemented in cppassist as well: https://github.com/cginternals/cppassist/blob/master/source/cppassist/include/cppassist/fs/FilePath.h

scheibel · 2018-02-22T17:29:43Z

source/tools/glkernel-cli/helper.cpp

+}
+
+bool canBeVec2(const cppexpose::Variant & v) {
+    const auto arr = v.asArray();


This conversion may fail, at least assert against nullptr.

If this returns a nullptr we return false, since this conversion did not work.

scheibel · 2018-02-22T17:36:39Z

source/tools/glkernel-cli/helper.cpp

+  float min = std::numeric_limits<float>::max();
+  float max = std::numeric_limits<float>::min();
+  const auto end = kernel.cend();
+  for (auto it = kernel.cbegin(); it != end; ++it)


I suggest adding a const-overloaded begin and end to tkernel and use a range-based for loop instead:

for (const auto & p : kernel) { const auto x = p.x; const auto y = p.y; //... }

scheibel · 2018-02-22T17:43:31Z

source/tools/glkernel-cli/KernelObject.h

+{
+public:
+    Kernel4Object(int width, int height, int depth);
+	glkernel::kernel4& kernel();


Please switch to 4 spaces instead of tabs.

scheibel · 2018-02-22T17:48:25Z

source/tools/glkernel-cli/JsonImporter.cpp

+
+#include <cppassist/logging/logging.h>
+
+void forEachCell(cppexpose::VariantArray * depthArray, std::function<void(const cppexpose::Variant&)> lambda)


I suggest you provide an forEachCell overload where the lambda takes the current index as second parameter.

scheibel · 2018-02-22T17:49:22Z

source/tools/glkernel-cli/JsonExporter.cpp

+
+
+std::string JsonExporter::stringify(const cppexpose::Variant &array) {
+    auto outputMode = m_beautify


The outputMode may be const, too.

scheibel · 2018-02-22T17:51:39Z

source/tools/glkernel-cli/JsonExporter.cpp

+    auto outputMode = m_beautify
+                      ? cppexpose::JSON::OutputMode::Beautify
+                      : cppexpose::JSON::OutputMode::Compact;
+    return cppexpose::JSON::stringify(array, outputMode);


Once cginternals/cppexpose#56 is implemented, you should use the interface writing to a stream.

scheibel · 2018-02-22T17:53:23Z

source/tools/glkernel-cli/CMakeLists.txt

+    JsonImporter.h
+    PngExporter.h
+    helper.h
+    )


We choose to indent the closing brace as much as the line containing the opening one. In this case: no identation.

Update readme: add cli-tool manual

…' into cli-tool

j-beyer and others added 30 commits November 20, 2017 15:55

Create new cli project with CMakeLists.txt

aa91280

Merge branch 'master' into cli-tool

cbe7187

Add basic command line flag handling using cppassist/cmdline

e3b3391

Merge branch 'fix-openmp-check' into cli-tool

c2f92a5

Fix indentation in main.cpp

56b626a

Require cppexpose dependency

729a8c9

Rewrite CLI using cppassist::CommandLineProgram

5eed0a2

Remove unused import, add version

b56cc35

Use switch instead of different actions

0905f9f

Merge pull request #11 from p-otto/cli-commandlineprogram

f34a3b2

Rewrite CLI using cppassist::CommandLineProgram

Merge branch 'cli-tool' into json-export

4bc9d15

Start basic scripting

e26abe5

Merge branch 'cli-tool' of https://github.com/p-otto/glkernel into cl…

e7a8b5d

…i-tool # Conflicts: # source/tools/glkernel-cli/main.cpp

Remove old code

0fd5e2f

Implement conversion between kernel and variant to enable javascript …

b8f51a7

…to access kernel

Incorporate change ideas from #1

45bfa23

Extract methods for readability

bcd44d3

WIP new scripting interface

c412093

Merge pull request #23 from p-otto/22-cli-api-changes

8f89211

#22 cli api changes

Merge branch 'cli-tool' into json-export

a11f8ee

Add KernelGenerator and KernelExporter for kernel generation from jav…

720aaa6

…ascript, and kernel export to JSON

Add --force switch and abort if output file exists and --force is not…

4cd1171

… given

Add comment explaining empty short name

168a537

Incorporate review feedback

29dfae8

Merge pull request #24 from p-otto/13-no-files-overridden

d10baa9

Add --force switch

Merge branch 'cli-tool' into json-export

261d897

Remove force parameter from exporter - is handled before now

003c909

WIP: Expose glkernel operations to JavaScript

12ce279

Working: - Creating kernels from JS (currently only kernel<float>) - Calling e.g. kernel.sequence.uniform() Missing: - Expose more functions

Further WIP on exposure

0cfba92

- Added scale::range and sort::distance - Figured out JS default parameters (see sort.distance) - Started JS-sided detection of parameter type

Rewrite kernel json export - unwrap kernel variant, and append values…

bf06020

… to array

Philipp Otto and others added 3 commits February 22, 2018 14:10

Throw if glkernel.js or input js file could not be located

3a7d17d

Move glkernel.js to data folder

cd19c8f

Merge pull request #41 from p-otto/scripting

0fcad02

Scripting

j-beyer mentioned this pull request Feb 22, 2018

Remove glkernel-cmd #26

Open

Philipp Otto and others added 8 commits February 22, 2018 15:13

Return kernel object in every js function call to remove need to retu…

b9076d6

…rn reference at end of file

Cleanup helper.h by moving some functionality for PNGExporter and mov…

95e311b

…ing implementation to cpp file

Remove unnecessary template

3bae4ee

Remove duplication between JSInterface and JsonImporter

9d3d5df

Add cli-tool to readme

943bc46

Add $ to bash commands, highlight TODO

382ed2c

Merge pull request #55 from p-otto/cleanup_helper

53d774e

Cleanup helper.h

Merge pull request #54 from p-otto/return_kernel_object

56c3bfb

Return kernel object in every js function

scheibel reviewed Feb 22, 2018

View reviewed changes

Tobias Knöschke and others added 14 commits February 26, 2018 13:45

Write generated .js to different directory

d1c2586

Change how #includes are generated

b12f123

Ease renaming generate target

8b574f4

Merge pull request #57 from p-otto/43-update-readme

502b8ec

Update readme: add cli-tool manual

Merge branch 'master' into cli-tool

7386f5f

Fix compiler warning

397a73e

Fix formatting in helper

23908a4

Re-enable chaining in user JS code

c49e096

Use cppassist for extension detection

84833e3

Use range based for loop

49ec53c

Allow range based for loop for const kernels

4e9ed09

Adressing review comments

badae3d

Merge remote-tracking branch 'origin/restrict_template_instanciations…

59e52b1

…' into cli-tool

Generator: change to cppassist::error()

74c23fa

j-beyer changed the title ~~WIP: glkernel cli-tool~~ glkernel cli-tool Mar 30, 2018

j-beyer changed the title ~~glkernel cli-tool~~ Introduce new cli-tool Mar 30, 2018

cgcostume merged commit 524f951 into cginternals:master Apr 19, 2018



		std::string extractInputFormat(const std::string & inFileName) {
		if (inFileName.find('.') == std::string::npos)


		#include <cppassist/logging/logging.h>

		void forEachCell(cppexpose::VariantArray * depthArray, std::function<void(const cppexpose::Variant&)> lambda)



		std::string JsonExporter::stringify(const cppexpose::Variant &array) {
		auto outputMode = m_beautify

Conversation

j-beyer commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

j-beyer commented Feb 12, 2018 •

edited

Loading