3dcitydb / importer-exporter

3D City Database client for high-performance import and export of 3D city model data
Apache License 2.0
126 stars 53 forks source link

Seeking input on the redesign of the command-line interface #139

Closed clausnagel closed 3 years ago

clausnagel commented 4 years ago

The current CLI just offers command-line parameters for the operation to be executed, for instance, -import, -export or -kmlExport. All further settings such as database connection details or export filters must be provided in an external config file.

We want to redesign the CLI. The main goal is to provide more command-line parameters to be able to trigger default operations without the need for an additional config file. This should help to simplify the use of the CLI. Another goal is to allow plugins to programmatically extend the CLI with new operations.

Some initial thoughts:

For example, invoking a CityGML export using a bbox filter could look like this:

> impexp export --db-host=localhost --db-name=citydb --db-user=user --bbox=1,2,3,4 my_citygml_file.gml

Well, it's obvious that we cannot map every option from the Importer/Exporter preferences to a command-line parameter. So, it must still be possible to provide a config file for these settings. But it should be optional and if omitted, default values should be chosen instead.

Which parameters would you like to see for the CLI? For example, is a --bbox parameter for a CityGML export like in the above example sufficient or do you also need more parameters to specify the bbox mode (overlap, inside), the CRS used for the bbox, or even a tiling based on the provided bbox?

Son-HNguyen commented 4 years ago

Thanks for the init Claus. The GUI offers an intuitive way to interact with the software but the CLI is definitely a big plus and I'm all for it. Personally I use CLI when I'd like to do something quickly or when I want to automate something, so to be able to put some settings in a config file will help a lot (and it makes the commands shorter too). As for the CLI parameters, I'm thinking whether we could use the same options based on the XML config file stored for the client as a starting point (not necessarily all of the options there) and extend it along the way if needed? Or could the same XML config file also be used for the CLI (might be tricky)? What do you think? Is there an online place where we could list the parameters together and edit them later if changes are needed?

yaozhihang commented 4 years ago

@Son-HNguyen Do you mean to introduce a simplified version (a subset) of the current XML config for CLI?

clausnagel commented 4 years ago

Well, I would definitely avoid having two different XML configs. My idea is that you can provide a config file through the --config option like in the current version of the CLI. The XML structure must follow the default config file project.xml. You can omit any unnecessary XML elements and keep it as small as possible (missing elements will then be populated with default values).

In contrast to the current CLI version, I think 1) providing a config file should be optional and 2) if a config file is provided then the command-line options should take precedence. Otherwise providing those additional command-line options is rather useless. In most cases, it should be fine to use the CLI without providing a config file.

Good question how to come up with a useful set of command-line options. The XML-based config is quite complex due to complex data types, nested elements and additional attributes. Just take the CityGML bbox export filter settings as example. Do we really need to map all possible settings to CLI options?

@Son-HNguyen No, we do not have an online place for the parameter discussion yet. What do you have in mind? Something like a Google spreadsheet?

yaozhihang commented 4 years ago

In my opinion, the --bbox=minx,miny,maxx,maxy,srid should be sufficient enough for CityGML bbox filter in CLI. Because:

  1. This is in common use by CityGML Import, KML Export, and Spreadsheet Export...
  2. All other filter bbox settings like mode (overlap, inside), tiling options etc. could be omitted. They sound like fine-grained settings and the default option All Overlapping Features is suitable in most practical cases.

logging settings -> good for CLI

Introducing database connection parameters in CLI is definitively very useful and helpful. The current encoding of database password in xml config as plain text is not friendly for CLI.

Son-HNguyen commented 4 years ago

The reason I thought about this XML config file is that if people, who are used to working with the GUI, occasionally have to execute the same process for some reason in CLI (such as for repeated/automated processes), then they do not need to get to know what all the parameters do and which options they should choose best. Just plug the existing config file in and they're good to go (such as: if no parameters are given in the CLI command, use the XML config file instead). For people that mostly work in CLI though, this would be useless. So it's definitely right to make the config file optional (I know there are people that take pleasure in typing the parameters in the command themselves :highfives:).

Of course parsing the XML config file might be very tricky and time-consuming to implement, since we really don't need that much information in a nested structure. I agree that if we took the XML config file as input for CLI, it should be kept minimal (or the CLI does not need to consider the entire file). We could say we take the options in the XML config file as an inspiration and only consider the most relevant ones. But if this turns out to be way more complicated than expected (which it very well might), then we could use a simpler config file for the CLI commands.

@yaozhihang I agree, the bbox parameters look clean and logs would be very helpful too!

@clausnagel We could use Trello for organizing stuff. I personally have good experience with it, but I understand others might not want to register for yet another tool. I could create one Trello board for all of us and public viewers if you and Zhihang would like to.

clausnagel commented 4 years ago

Thanks for your input, @Son-HNguyen. Using an XML config file in the way you describe is possible with the current CLI for exactly the reasons you describe. And I think we all agree that this has to be possible with the reworked CLI as well.

The reason for this issue is that the current CLI only works if you provide an XML config file. So, you have to create this XML file beforehand (programmatically or manually using the GUI). In my opinion, this is rather a drawback if you think of automated processes. I would like to be able, for instance, to run a Docker image and just provide a few parameters to export a portion of the model stored in the 3DCityDB.

clausnagel commented 3 years ago

With commit 13a5de56f8f0f17b0bc631223490e15b5e7cf80a the cli-rework branch now provides a first glimpse of how the new CLI will look and feel like. The gui command is already fully implemented and functional. And plugins can already register their own additional commands. The rest requires much more work... :-)

Try the following:

$ impexp -h           (prints a general usage help)
$ impexp gui -h       (prints the usage help for the gui command)
$ impexp gui          (runs the gui command)
$ impexp export -h    (prints the usage help for the export command)
BWibo commented 3 years ago

Hey there, this is especially important when containerizing the 3DCityDB stack and bring it to the cloud. Here are some thoughts regarding the usage of the CLI from a container (e.g. Docker).

Configuring applications running inside containers

In general, applications running in containers are most commonly configured using environment variables, cli switches, args and configuration files. The first three are provided during container startup (e.g. docker run -e ENV_VAR=VALUE image:tag application -switch1 arg1) the latter however is rather unhandy, as you need to bring the configuration file to the file system of the container in some way. This can be done by mounting a file/directory from the host system, providing a secret (e.g. docker secret configFile config.xml) to the container or some other way, like e.g. an URL specified as an env_var where the configuration can be accessed by the container. Hence, in my opinion we should try to make as much options as possible accessible using CLI switches or by having the Importer/Exporter ready ENV_VARs, as they are most easy to access. Regarding configuration files, there should be a default location (or switch to provided a pth) so using secrets is easily possible. A possible solutions to avoid providing config files are templates, that can be selected using a switch or env var. This could e.g. be helpful for options regarding styles for kml exports.

CLI switches

Essential switches are:

Nice to have for more complex cases:

So much for now, I will update this when there is more comming to my mind.

clausnagel commented 3 years ago

Thanks @BWibo, this is valuable input! The current picocli lib we are using for building the CLI supports:

I would also like to use CLI options and parameters as much as possible. I had not thought about environment variables before (only as one way of providing the password for the database connection, like --db-password:env=ENV_VAR_NAME). From your docker run example, it seems that providing environment variables is just like using CLI switches. So I would prefer to focus on CLI switches now, also because picocli has no built-in support for environment variables. So picocli does not automatically search for an environment variable if a CLI switch has not been provided - something we would have to code ourselves.

There will be a --config option to load an existing full-blown configuration file. But in contrast to the previous version, it will not be mandatory anymore.

Yes, I also want to have a --query switch that can take an XML-encoded query expression. There might be limitations of the length of the command line in some enviroments. The current picocli solution for this is using @-files. This is just a file containing CLI options and parameters, and then you provide the path to this file on the command line.

For instance, the @-file might contain:

--log-level=info
--query=<query>... very long query ... </query>

You can then use it simply like:

$ impexp export @/path/to/argument/file

You can use multiple @-files in one call, and even mix them with CLI options on the command line. But, as a drawback, it's file-based again.

What is your idea with the template style files? I am not sure that I got this idea correctly.

BWibo commented 3 years ago

Hey there, I'm not sure if the @-files will work with Docker. As far as I know the file would have to be on the file system of the container. Is the @file option already implemented? If yes, I can run some basic tests using https://github.com/tum-gis/3dcitydb-importer-exporter-docker.

Regarding the templates I am thinking of a a mechanism that allows users to provide common configuration patterns, e.g. for a certain company or use-case as a template file that can e.g. be included in the container during build time and then be selected using a switch or env var. Templates can contain anything, e.g. a frequently used export configuration, a configuration of kml export styles for a feature type. This is currently possible by providing full config files and and select one of them during runtime. However, the full xml config is huge and hard to edit and maintain. As dicussed before, I agree we should avoid ending up handling different config files, but maybe it is possible to overwirte certain sections of a config with the content from such templates. I'm sure, there are other options to provide such templates to a container as well that avoid having to build your own images.

This would give users the ability to customize there importer-exporter images without having to understand and maintain the entire xml config file. Moreover, we could provide template files for common use-cases with the official image out of the box.

Here is an example:

  1. Clone importer-exporter docker repo and add your templates.
./impexp-docker
   ./template
       ./kmlExportBuildingsStyleX_ProjectA.xml
       ./citygmlExport_ProjectB.xml
       ...
   ./Dockerfile
  1. Build image, the templates are automatically included. docker build -t myCompany/impexp .

  2. Run container and select template to use:

docker run -e template=citygmlExport_ProjectB.xml myCompany/impexp or docker run myCompany/impexp --template=citygmlExport_ProjectB.xml

clausnagel commented 3 years ago

Ok, so templates are (partial) config files inside the container? And --template=<template> is also "just" a pointer to a template file? The difference is that the template files are available easily because they are built into the docker container?

If so, we need some priority rules, for example:

  1. First, it is checked whether a config is provided via the --config option. If so, it's settings will be loaded. Otherwise, an empty config (and thus default settings) will be used instead.
  2. If a --template option is provided, then this template is loaded afterwards. Its settings will override correspdoning settings from 1.
  3. If a user provides more fine-grained options on the comand line (for instance --bbox or --query, those settings will override the settings from both 2. and 1.

Does this make sense?

Currently, there is no support for loading partial config settings. We would need to define reasonable chunks of the config that should go into separate template files.

clausnagel commented 3 years ago

Sorry, missed one of your questions, @BWibo. Yes, @-files are already supported in the cli-rework branch. Btw, I guess one could also compile such files into the docker container like templates. Then it should also be possible to easily reference them from the command line.

BWibo commented 3 years ago

Yes, exactly. Templates could be partial config files that live inside the container and are referenced using a pointer, which can be e.g. a switch/env var. The config hierarchy you propose makes totally sence to me.

The main difference is that template files are more lightweight and user-friendly than the full config.xml. They have to be designed once for a specific purpose/project (maybe from the importer exporter gui) and are subsequently available in the container to be used whenever they are needed.

However, I'm not sure if it is worth the effort implementing this, as we can have the same mechanism using full config files and the --config switch.

OK, I'll take a look at cli-rework and test the @files option.

BWibo commented 3 years ago

I build an image from cli-rework to run some tests, it is available here: tumgis/3dcitydb-importer-exporter:cli-rework.

I placed a @file containing the -V option /share/config/imp-exp-opts.txt inside the image.

The behavior is as expected: When running docker run -it tumgis/3dcitydb-importer-exporter:cli-rework @/share/config/impexp-opts.txt the file will be read and the command line options inside the file are parsed by the impexp running inside the container. When I run docker run -it tumgis/3dcitydb-importer-exporter:cli-rework @impexp-opts.txt with impexp-opts.txt locally available in the working directory the impexp cannot see the file and returns an error:

Unmatched argument at index 0: '@impexp-opts.txt'
Did you mean: export?

Mounting the file from the host system will ofcourse works as well:

docker run -it --rm \
  -v "$(pwd)/impexp-opts.txt:/test/opts.txt" \
tumgis/3dcitydb-importer-exporter:cli-rework @/test/opts.txt

Hence, any file we want to use for configuration has to be compile inside the container during build, mounted from the host system, or brought to the file system of the container in some other way, e.g. by providing a URL and downloading it. This is unhandy, as we need to bring these files to the host system first (which is usually remote). That's why template files inside the container increase usability.

PS: There is another important CLI option we should provide:

BWibo commented 3 years ago

PS: @Son-HNguyen did some work on the auto bbox feature in https://github.com/3dcitydb/importer-exporter/tree/bbox_cli. As far as I know this only covers kmlExport and does not allow for specifiying feature types to filter features that are used for the bbox clacluation.

clausnagel commented 3 years ago

Thanks for testing, @BWibo. Good to see that the @-files and the --config option generally works from a Docker image.

We moved the auto-bbox-calculation to issue #138 because this is not just a CLI topic but should also be available in the GUI.

clausnagel commented 3 years ago

I have added more query and filter options to the export command in the cli-rework branch. For example, to export all buildings from a local database using a bbox filter, simply use the following command. Note that --db-password is used without a value. This means that you will be prompted for the password on the command line.

$ impexp export --db-host=localhost --db-name=citydb --db-username=user --db-password --type-names=Building --bbox=1,2,3,4 --output=/path/to/output.gml

Please test and let me know your thoughts.

@BWibo: You can provide JVM options via the environment variable JAVA_OPTS. For example, you can define memory settings (e.g. via -Xmx=n) and provide the number of cores (-XX:ActiveProcessorCount=n). Does this cover your idea of performance parameters ? Or do you also want to be able to set the number of minimum and maximum threads like in the GUI preferences dialog?

BWibo commented 3 years ago

Hey there, I build a fesh image (tumgis/3dcitydb-importer-exporter:cli-rework) and did some testing.

I tested db connection parameters, bbox, bbox-mode, XML query, SQL query and setting appearance theme. That seems to work fine so far and it is already pretty powerful. Big thank's for these options! KML export is not jet implemented, right?

Setting performance parameters using JAVA_OPTS works fine, as expected. I was thinking about setting the threading options as provided in the GUI, to make sure the impexp container can leverage the cores I let the container use.

The @files option is really handy. I can easily setup reproducible test cases using these files.

clausnagel commented 3 years ago

Thanks for testing @BWibo, sounds great.

Yes, the export command is only intended to export data in CityGML format. There will be an extra command to export data as KML/COLLADA/glTF. This command was called kmlExport in the old CLI. But since this name only refers to KML, one idea is to rename it to export-vis (for visualization) in the new CLI.

But what are reasonable CLI options for this command (besides database options and query options like bbox, type names, etc.)? For instance, how to define the "display as" and "visible from" options? I assume additional styling options will be too complex for the CLI as they must be provided for each feature type in the config file. In general, the KML/COLLADA/glTF settings in the config file are very complex. So, it would be good to also get suggestions from @yaozhihang and @Son-HNguyen on this.

I will work on the import and validate commands in the meantime.

Son-HNguyen commented 3 years ago

To handle the many binary options (true/false) or options with a clear list of values to choose from in the KML/COLLADA/glTF settings we could use some predefined identifiers to mark them (such as the numbers in the command chmod 777, where each of the numbers here defines a unique combination of the option values of e.g. one type of objects). These numbers can be looked up by users in a definition table defined by us. This look-up table must not be changed frequently though, so it's a thing to consider for future maintenance.

To handle the options containing arbitrary numeric values, I guess they must be given either directly in the parameters or in a config file. But I think there are not as many such options compared to the binary options or options with a list of fixed values as mentioned above.

Just my quick thought, I might oversee something. What do you think @clausnagel @yaozhihang?

BWibo commented 3 years ago

One more thing regarding kmlExport: The collada2glTF tool location is currently specified, with a path. This path and the configuration options of the tool should be easily accessible with command line switches.

BWibo commented 3 years ago

I just stumbled on this problem by accident. I just created a fresh build of the docker image from 87f276de6b3089a644877327cffb911ad7fd3023. I am trying to export serveral tiles bound by polygons using xmlQuery without appearances:

The xmlQueries look like this (there are more than 300 of them):

<query xmlns="http://www.3dcitydb.org/importer-exporter/config">
  <typeNames>
    <typeName xmlns:bldg="http://www.opengis.net/citygml/building/2.0">bldg:Building</typeName>
  </typeNames>
  <filter>
    <intersects>
      <valueReference>bldg:boundedBy/bldg:GroundSurface/bldg:lod2MultiSurface</valueReference>
      <polygon srid="4326">
        <exterior>-73.84792 40.87134 -73.84725 40.870987 -73.84699 40.870853 -73.846405 40.870556 -73.84609 40.870388 -73.84578 40.870228 -73.84517 40.869915 -73.84488 40.86976 -73.84442 40.869522 -73.84423 40.86942 -73.84358 40.869087 -73.843155 40.868862 -73.842804 40.86868 -73.84241 40.868473 -73.84185 40.86819 -73.841225 40.86786 -73.840645 40.867554 -73.839745 40.86709 -73.83955 40.866985 -73.83922 40.86682 -73.83875 40.866573 -73.837845 40.8661 -73.83697 40.865643 -73.83679 40.865547 -73.83619 40.865223 -73.835785 40.865017 -73.8316 40.862644 -73.83152 40.8626 -73.830894 40.862213 -73.830124 40.86205 -73.82973 40.861954 -73.829506 40.861843 -73.82924 40.861633 -73.82912 40.86151 -73.82842 40.86093 -73.82835 40.860893 -73.82843 40.86079 -73.82882 40.86045 -73.82945 40.86 -73.82961 40.85989 -73.829735 40.85981 -73.82987 40.85974 -73.83002 40.85968 -73.83017 40.859634 -73.83103 40.859344 -73.83191 40.85907 -73.833084 40.85868 -73.83349 40.85853 -73.83437 40.858192 -73.835335 40.85783 -73.83629 40.857475 -73.83682 40.857277 -73.83724 40.857357 -73.83817 40.85751 -73.83911 40.85766 -73.84005 40.857807 -73.84097 40.85795 -73.84191 40.858093 -73.84329 40.85829 -73.84459 40.858444 -73.84553 40.85854 -73.84646 40.858635 -73.8474 40.85868 -73.848335 40.858704 -73.84931 40.85872 -73.85028 40.858658 -73.85123 40.858578 -73.85217 40.858498 -73.85311 40.85841 -73.85406 40.858322 -73.85585 40.858173 -73.855995 40.858162 -73.856636 40.858116 -73.85697 40.85875 -73.85713 40.859047 -73.85718 40.859123 -73.85722 40.859203 -73.857254 40.859287 -73.857285 40.85937 -73.857315 40.859455 -73.85733 40.85954 -73.857346 40.859627 -73.85735 40.85971 -73.85739 40.859985 -73.85746 40.860428 -73.857475 40.860497 -73.857475 40.860542 -73.85752 40.860847 -73.85753 40.860905 -73.85757 40.861164 -73.8576 40.86142 -73.85761 40.861492 -73.85767 40.861973 -73.85768 40.862034 -73.85769 40.862133 -73.8577 40.862186 -73.857704 40.86224 -73.85771 40.862278 -73.8578 40.863037 -73.85806 40.86304 -73.858444 40.86376 -73.85871 40.864254 -73.85891 40.864635 -73.85961 40.865513 -73.86055 40.86553 -73.861565 40.86555 -73.86154 40.866154 -73.8615 40.86755 -73.861496 40.867783 -73.861435 40.869602 -73.86138 40.871338 -73.86037 40.87132 -73.85943 40.8713 -73.85849 40.871284 -73.85755 40.871265 -73.85661 40.87125 -73.85655 40.87303 -73.856514 40.874218 -73.855774 40.873985 -73.855515 40.873905 -73.85451 40.87358 -73.85364 40.873302 -73.85358 40.873276 -73.85266 40.872993 -73.85171 40.872696 -73.85108 40.872494 -73.85079 40.872395 -73.84988 40.8721 -73.84894 40.871784 -73.848595 40.87167 -73.84792 40.87134</exterior>
      </polygon>
    </intersects>
  </filter> 
</query>

This is the code I use to iterate over the xmlQuery files in a folder:


find fme_output/ -type f -name "*.xml" -exec \
  docker run -it --rm --name impexp \
    -v /d/lca_tga/nyc/tiling/:/share/data/ \
    tumgis/3dcitydb-importer-exporter:cli-rework export \
    -H ssd -d nyc_bldg -u postgres \
    -o "/share/data/exports/{}.gml" "@/share/data/{}" \;

The problem is: When I add the --no-appearance switch, I get an error that this switch and xmlQuery are mutually exclusive. I could not find anything in the xmlQuery documentation on how not to export appearances. Leaving the <appearnce> or <theme> elements empy gives me this error: No valid themes provided for appearance filter.

Is there a way to not to export appearances at all using xmlQuery? If no, --no-appearance should be usable with -q.

PS: I was able to resolve this adding a config file with the no-appearance option set, however, this should be possible without the config file.

clausnagel commented 3 years ago

Ha, good catch. --no-appearance should in fact be usable with -q.

clausnagel commented 3 years ago

I added the import, delete, validate and export-vis commands to the new CLI. Just update your cli-rework branch to get the latest changes. Imho, a first version of the new CLI is complete now.

@BWibo:

@Son-HNguyen:

BWibo commented 3 years ago

That's really cool! I built a fresh image from cli-rework and pushed it to: tumgis/3dcitydb-importer-exporter:cli-rework. You can use it for testing like this:

docker run --name impexp -it --rm \
   -v "/my/local/share/folder/:/share/" \
 tumgis/3dcitydb-importer-exporter:cli-rework SWITCHES ARGS @FILES

Note: Anything file related must use the locally mounted folder but the path as seen from inside the container. E.g. when you want to export a file use .... tumgis/3dcitydb-importer-exporter:cli-rework export -o /share/myGmlFile.gml to export the file to /my/local/share/folder/myGmlFile.gml.

I hope I'll find some time for testing next week.

Son-HNguyen commented 3 years ago

For the KML/COLLADA/glTF export we could do it similarly to your implementation for the CLI in general. The command signature could look like this:

export-vis [bbox] [export_options] [general_options] [rendering_options] [balloon_options] [altitude_options] [config_file]

If the user did not specify any of the [general_options], [rendering_options], [balloon_options] and [altitude_options], these settings will be set to default values (the same as those default values stored in the GUI). If [config_file] is available, the settings stored in this file shall be used instead.

If some configurations from the [general_options], [rendering_options], [balloon_options] and [altitude_options] are explicitly given in the command line, they will override the default values or those from the config file.

So the priority order (from lowest to highest) would be:

default values < config file < explicit parameters

The following parameter lists are not exhaustive, the parameters that are not listed will receive default values or those from the config file.

The [export_options] could include:

Parameter Description Value Example
footprint export footprint number of pixels as visible from --footprint=120
extruded export extruded number of pixels as visible from --extruded=120
geometry export geometry number of pixels as visible from --geometry=120
daegltf export COLLADA/glTF number of pixels as visible from --daegltf=120
theme appearance/theme --theme=visual
features feature types to export Bridge brid, Building bldg, CityFurniture frn, CityObjectGroup grp, Generics gen, Landuse luse, Relief dem, Transportation tran, Tunnel tun, Vegetation veg, WaterBody wtr --features=brid,bldg

The [general_options] could include:

Parameter Description Values Example
collada2gltf path to the COLLADA2GLTF converter --collada2gltf=/app/collada2gltf
the options allowed by the COLLADA2glTF converter link
kmz export in KMZ instead true or false --kmz

The [rendering_options] could include:

Parameter Description Values Example
orientation consider surface orientation true or false --orientation
normals generate surface normals true or false --normals
crop crop texture images true or false --crop
texAtlas generate texture atlases algorithms basic, tpim1, tpim2 (without image rotation) --texAtlas=basic

The [balloon_options] could include:

Parameter Description Values Example
description placemarks must include <description> true or false --description
balloonSrc set balloon content source :gen for generic attribute Balloon_Content, or path to file, or :gen,path to select file only when no generic attributes are available --balloonSrc=:gen,/app/file

The [altitude_options] could include:

Parameter Description Values Example
originalZ use original z coordinates without transformation true or false --originalZ
altMode altitude mode relative, absolute, clamp --altMode=absolute
altOffset altitude offset No offset if 0, constant offset if number, move objects to bottom height 0 if bottom, or use generic attribute GE_LoDn_zOffset if gen --altOffset=gen
gapi use Google Elevation API when no generic attribute GE_LoDn_zOffset is available --gapi=url

The [rendering_options] and [balloon_options], etc. should of course only apply to the feature types defined by the parameter features in the export_options.

That's a lot of parameters but I think this way it is more manageable than using the numbers such as in chmod 777 I suggested before. @clausnagel @yaozhihang @BWibo Feel free to add if I forgot something.

clausnagel commented 3 years ago

Hi @Son-HNguyen, thanks for your proposals. Have you checked the current implementation?

Son-HNguyen commented 3 years ago

@clausnagel I have not (just saw today that there is a PR on this branch). Will this cause conflicts with the PR?

clausnagel commented 3 years ago

Maybe you can build a version from the cli-rework branch (or use the docker provided by @BWibo), and then run impexp export-vis -h.

Some of your proposals are already implemented, but some in a slightly different way. So, no, there is no general conflict. Some of your proposals have not been implemented yet. so it's good input. In the GUI, things like your [rendering_options] are available per feature type. Do you suggest that, for example, --normals should rather be applied to all features to be exported?

Son-HNguyen commented 3 years ago

Thanks @clausnagel for the heads up. Sorry was slightly out of the loop in this thread. I just have a closer look at the parameters in the current CLI and it looks very similar to what I imagined. 👍 I think the option --normals should be applied to all feature types defined by the parameter --features defined in [export_options]. A customizable option for each different feature type would make the parameter lists explode I suppose? If the user would like to e.g. enable normals for buildings and bridges but not for tunnels, he/she could first export buildings and bridges with normals enabled, then export tunnels without normals. What do you think?

clausnagel commented 3 years ago

Thanks for checking the current implementation. I agree that it is quite similar to what you proposed. I would therefore keep the currently implemented parameters wherever they serve a similar purpose.

Yes, this is exactly the way I would suggest to apply --normals. Ok, I will check [rendering_options] and see whether I can add support for them. Are [altitude_options] also required for the command line? Or wouldn't it be sufficient to pass them using a config file?

I tend to skip [balloon_settings] though - at least from a first version. I don't think KML balloons are the most important feature, so providing a config file for this use case should be fine. And you need highlighting to be activated for them to work. So more parameters are required...

One more remark: I suggest to wait for the review of the current PR before adding more and new stuff to it.

Son-HNguyen commented 3 years ago

Yes I thought the same, [altitude_options] and [balloon_options] are currently not as prioritized. Wanted to not include them in my post but for the sake of completeness, I just went ahead and post them. ;-)

I agree with waiting for the review first. Hopefully I can the find the time in the next days to review it also.

clausnagel commented 3 years ago

Thanks and kudos to all involved in the discussion and development of the new command-line interface. It has been merged to master in the meantime (#147, #151) and will be available with the next release.