Add verticalScale support and metadata convert post processor - #155
Add verticalScale support and metadata convert post processor#155zhak5388 wants to merge 3 commits into
Conversation
b93e47f to
b639e8d
Compare
[Maven Build Status]📑 Commit: 📦 Download artifact: Generator.jar |
62638c0 to
459710c
Compare
pyrollo
left a comment
pyrollo
left a comment
There was a problem hiding this comment.
Déjà quelques commentaires
746b6db to
1394827
Compare
6377a05 to
4eb50bd
Compare
|
Je viens de push un commit qui fusionne les classes Edit: Commentaire déprecié. Deux classes au lieu d'une. |
9b17a1c to
10ba5a0
Compare
There was a problem hiding this comment.
Bon au final, beaucoup d'aller retour! Au final, je suis sur quelque chose d'assez proche dans l'esprit de la version fds, avec quelques différences.
MapToWorldConverterqui a des méthodes pour faire de la conversion de distance et de coordonnées etWorldToMapConverteruniquement coordonnées. Je trouve que c'est plus clair en terme de responsabilité (il y a un peu de repetition mais je trouve que c'est mieux qu'avoir une seule classe converter qui pourrait avoir des instances avoir des méthodes non appropriées).Generationn'est plus responsable de créer les objets de transformations.- Ajout de petits commentaires / javadoc pour clarifier certains points qui pourraient être peut-être poser problème dans le futur. (Unités CRS, CRS projeté, altitudeOffset pour différence altitudes entre CRS et/ou pour palier limite Minecraft en hauteur, échelle de déformation verticale pour Minecraft, necessité de travailler en unité de voxels pour les taches de rendus notamment). J'ai aussi ajouté deux TODO:
- Dans
PopulateHeightmapTasksur le faitFloatMatrixModelsoit en x/y en coordonnées du monde et contiennent des données qui ne sont pas en voxels (Pas sur de faire la conversion dans le model car c'est possible de contenir des données qui ne sont pas de l'altiude et donc ne necessite pas de conversion) - Dans
MapToWorldConvertersur le fait que l'altitude offset pourrait contenuir différence altitudes entre CRS ou pas
- Dans
Ci-dessous quelques remarques/questionnement
| if (type.isInstance(value)) | ||
| return model; |
There was a problem hiding this comment.
Ça serait peut-être utile de déplacer ce test dans ValueParser.
Concernant le fait d'avoir ValueParser qui puisse parser une donnée en tant que distance vertical à la place d'un post processeur de conversion, je n'ai pas pleinement exploré cette possibilité mais ça ne me semblait pas aussi trivial, j'ai l'impression que ça implique changer les autres PostProcessor pour faire en sorte qu'il y ait une fonction de model.
Aussi j'ai l'impression que les PostProcessor auront besoin d'un changement à un moment (Modifier un Model juste pour les besoins d'une tache, ou bien simplement l’écriture des failures policy (Par exemple actuellement on est obligé de répéter ifMissing à tous les post processors dans le fetchData)
Ceci dit, la solution actuelle (le fait d'introduire un autre PostProcessor) fait un minimum de modification mais ça ajoute de potentielles modifications futures, donc pas sur que ça soit idéal.
| // Checking units once | ||
| Unit<?> unitMap = CRSUtilities.getUnit(mapCrs.getCoordinateSystem()); | ||
| Unit<?> unitWorld = CRSUtilities.getUnit(worldCrs.getCoordinateSystem()); | ||
| if (!unitMap.equals(unitWorld)) | ||
| System.out.printf("WARNING: the two CRS do not use the same unit. They might be awry results when converting distances. Unit map: %s, Unit world: %s%n", unitMap.getName(), unitWorld.getName()); |
There was a problem hiding this comment.
Alors initialement j'avais mis ce warning uniquement dans méthodes convertAltitude(), convert...Distance(), et mettre le warning dans le constructeur une bonne fois pour toute ça parait être une bonne idée et ça permet de ne pas stocker les CRS.
Sauf que, souvent on va avoir un map CRS (dans nos tests notamment) en degré , ça va faire un warning et ça pollue pas mal, du coup je suis tenté de virer tout simplement ce test. (Je me dit que la javadoc est assez explite sur le fait de la nécessité d'avoir les mêmes unités et ça me semble suffisant)
Je voulais aussi ajouter un test pour vérifier si le CRS cible utilise bien un système de coordonnées cartésien (car nécessaire pour les transformations affines) mais en suivant la même logique, la javadoc est assez explicite là-dessus.
Du coup, ça fait un peu un pas en avant et un peu en arrière (Je garde juste les commentaires/javadoc que du doc)
| this.transform = transform; | ||
| // The scales might be passed to the transform object, but it lost during creation | ||
| // The same value should be passed explicitly as it is required for convertAltitude(), convertHorizontalDistance(), convertVerticalDistance() | ||
| this.horizontalScale = horizontalScale; | ||
| this.verticalScale = verticalScale; | ||
| this.altitudeOffset = altitudeOffset; |
There was a problem hiding this comment.
Sur ce constructeur.. j'aurai bien aimé avoir un constructeur privé/protégé qui fait que l'initialisation et un public qui soit spécifique appelant le privé mais j'ai été bloqué avec le fait de devoir avoir this() en premiere ligne et laisser tomber la gestion d'erreur
| */ | ||
| public MapToWorldConverter(AffineTransformation preTransform, MathTransform crsTransform, AffineTransformation postTransform) { | ||
| this(new Converter(preTransform, crsTransform, postTransform)); | ||
| public WorldCoords2d convert(MapCoordinates2d coords) throws TransformException { |
There was a problem hiding this comment.
Ici j'ai hésité à faire un MapCoordinates2d convert(MapCoordinates2d coords) à la place. Cela permettrait de garder la précision et de laisser le soin à l'appelant de choisir comment arrondir.
En terme d'écriture ça semble plus judicieux d'avoir un MapToWorldConverter faisant MapCoord -> WorldCoord plutot que MapCoord -> MapCoord. (Peut être juste un soucis de nommage de MapCoord pour montrer qu'elle n'est pas exclusive à la carto.
Il y a pas cette gêne sur Geometry convert(Geometry geom). (Même contenant, mais c'est le contenu qui change)
J'ai le même questionnement pour WorldToMapConverter.
10ba5a0 to
650bc0d
Compare
650bc0d to
52ace42
Compare
2 participants
Description
Integration of the following commit from
fdsof PR#137:Notes
Convertertakes only oneMathTransformobjectTODOs
Self-checks
/docsfolder has been updateddocs/usage/Examples.mdwork the same (or have been adapted if subject to changes in this PR)