Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proj_create_crs_to_crs_from_pj(): add a ONLY_BEST=YES option (fixes #3533) #3535

Merged
merged 11 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions data/proj.ini
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ cache_size_MB = 300

cache_ttl_sec = 86400

; Can be set to on so that by default the lack of a known resource files needed
; for the best transformation PROJ would normally use causes an error. This
; default value itself is overriden by the PROJ_ONLY_BEST_DEFAULT environment
; variable if set, and then by the ONLY_BEST setting that can be
; passed to the proj_create_crs_to_crs() method, or with the --only-best
; option of the cs2cs program.
; (added in PROJ 9.2)
only_best_default = off
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to have this unset? The reason is for warnings. If this is set to ON/OFF, the warnings should be disabled similar to the ONLY_BEST option passed into proj_create_crs_to_crs_from_pj, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed with latest commit (gizz, all those accumulation of settings gets crazy complicated)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(gizz, all those accumulation of settings gets crazy complicated)

Perhaps a sign that some refactoring is needed in the area? There must be a smart way to do it without plastering the whole code-base with tripwires

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good if all problems had simple solutions :-)
To be honest, we're reaching a point where I'm losing interest spending more time working in this pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, we're reaching a point where I'm losing interest spending more time working in this pull request.

Hey now, I was not suggesting that you include a major refactoring in this PR. If this is a pain point during development it is worth considering a solution to the problem. It should obviously be done in a separate issue but I don't want to open the discussion unless there's an actual problem. Hence my question.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only axis of simplification I can imagine would probably be to merge the 2 warn and error boolean variables in a single one with 3 states: no warning (only_best=off), warning (no explicit setting), error (only_best=on).
There isn't nothing that is particularly difficult in the code. There are places in PROJ that are > 10x times harder to make sense of ( CoordinateOperationFactory::Private::createOperationsBoundToGeog() probably holds the record... Or perhaps CoordinateOperationFactory::Private::createOperationsCompoundToGeog():-) )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm glad we've de-escalated the situation from "crazy complicated" to "nothing .. particularly difficult" :-) My spidey-sense was tingling at the crazy complicated part - that sort of wording usually is an early warning that refactoring is in order. We don't always notice when knee-deep in the code so I just wanted to check whether this was an actual problem or not


; Filename of the Certificate Authority (CA) bundle.
; Can be overriden with the PROJ_CURL_CA_BUNDLE / CURL_CA_BUNDLE environment variable.
; (added in PROJ 9.0)
Expand Down
20 changes: 19 additions & 1 deletion docs/source/apps/cs2cs.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ Synopsis

| **cs2cs** [**-eEfIlrstvwW** [args]]
| [[--area <name_or_code>] | [--bbox <west_long,south_lat,east_long,north_lat>]]
| [--authority <name>] [--no-ballpark] [--accuracy <accuracy>] [--3d]
| [--authority <name>] [--3d]
| [--accuracy <accuracy>] [--only-best[=yes|=no]] [--no-ballpark]
| ([*+opt[=arg]* ...] [+to *+opt[=arg]* ...] | {source_crs} {target_crs})
| file ...

Expand Down Expand Up @@ -166,6 +167,23 @@ The following control parameters can appear in any order:
`south_lat` and `north_lat` in the [-90,90]. `west_long` is generally lower than
`east_long`, except in the case where the area of interest crosses the antimeridian.

.. option:: --only-best[=yes|=no]

.. versionadded:: 9.2.0

Force `cs2cs` to only use the best transformation known by PROJ.
`cs2cs` will return an error if a grid needed for the best transformation is missing.

Best transformation should be understood as the most accurate transformation
available among all relevant for the point to transform, and if all known
grids required to perform such transformation were accessible (either locally
or through network).

Note that the default value for this option can be also set with the
:envvar:`PROJ_ONLY_BEST_DEFAULT` environment variable, or with the
``only_best_default`` setting of :ref:`proj-ini` (:option:`--only-best`
when specified overrides such default value).

.. option:: --no-ballpark

.. versionadded:: 8.0.0
Expand Down
12 changes: 12 additions & 0 deletions docs/source/development/reference/functions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,18 @@ paragraph for more details.
- ALLOW_BALLPARK=YES/NO: can be set to NO to disallow the use of
:term:`Ballpark transformation` in the candidate coordinate operations.

- ONLY_BEST=YES/NO: (PROJ >= 9.2)
Can be set to YES to cause PROJ to error out if the best
transformation, known of PROJ, and usable by PROJ if all grids known and
usable by PROJ were accessible, cannot be used. Best transformation should
be understood as the transformation returned by
:cpp:func:`proj_get_suggested_operation` if all known grids were
accessible (either locally or through network).
Note that the default value for this option can be also set with the
:envvar:`PROJ_ONLY_BEST_DEFAULT` environment variable, or with the
``only_best_default`` setting of :ref:`proj-ini` (the ONLY_BEST option
when specified overrides such default value).

- FORCE_OVER=YES/NO: can be set to YES to force the ``+over`` flag on the transformation
returned by this function. See :ref:`longitude_wrapping`

Expand Down
9 changes: 9 additions & 0 deletions docs/source/resource_files.rst
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,15 @@ Its default content is:

cache_ttl_sec = 86400

; Can be set to on so that by default the lack of a known resource files needed
; for the best transformation PROJ would normally use causes an error. This
; default value itself is overriden by the PROJ_ONLY_BEST_DEFAULT environment
; variable if set, and then by the ONLY_BEST setting that can be
; passed to the proj_create_crs_to_crs() method, or with the --only-best
; option of the cs2cs program.
; (added in PROJ 9.2)
only_best_default = off
rouault marked this conversation as resolved.
Show resolved Hide resolved

; Filename of the Certificate Authority (CA) bundle.
; Can be overriden with the PROJ_CURL_CA_BUNDLE / CURL_CA_BUNDLE environment variable.
; (added in PROJ 9.0)
Expand Down
86 changes: 85 additions & 1 deletion src/4D_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,41 @@ int pj_get_suggested_operation(PJ_CONTEXT*,
return iBest;
}

/**************************************************************************************/
static void warnAboutMissingGrid(PJ* P)
/**************************************************************************************/
{
std::string msg("Attempt to use coordinate operation ");
msg += proj_get_name(P);
msg += " failed.";
int gridUsed = proj_coordoperation_get_grid_used_count(P->ctx, P);
for( int i = 0; i < gridUsed; ++i )
{
const char* gridName = "";
int available = FALSE;
if( proj_coordoperation_get_grid_used(
P->ctx, P, i, &gridName, nullptr, nullptr,
nullptr, nullptr, nullptr, &available) &&
!available )
{
msg += " Grid ";
msg += gridName;
msg += " is not available. "
"Consult https://proj.org/resource_files.html for guidance.";
}
}
if( P->ctx->warnIfBestTransformationNotAvailable )
{
msg += " This might become an error in a future PROJ major release. "
"Set the ONLY_BEST option to YES or NO. "
"This warning will no longer be emitted (for the current context).";
P->ctx->warnIfBestTransformationNotAvailable = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking that this warning may be useful to see in different operations. Thoughts about only showing this error once for each PJ* instead of once per PJ_CONTEXT*?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts about only showing this error once for each PJ* instead of once per PJ_CONTEXT*?

I'm slightly concerned, without numeric evidence, that there might be performance issues in situations where people use suboptimal transformations in an intensive way, while being happy with them. Logs might be overwhelmed with the warning. Once per context IMHO has the benefit of making attentive observers of logs that they might sub-optimaly use PROJ, while avoiding causing perf regressions.

Copy link
Contributor

@snowman2 snowman2 Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see that being an issue if we issued a warning every time. But, once per PJ* seems fairly reasonable to me as the same PJ* can be re-used. Additionally, if they are happy with the current mode, they have the option to set ONLY_BEST=OFF mode to improve performance.

However, your concerns are valid. If you want to do it once per PJ_CONTEXT*, I would remove the specific grids from the warning message to make a generic message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh well, I've added a commit to move the warning at the PJ* level rather than a the PJ_CONTEXT* one. We have some time before the next release to see how it flies...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

}
pj_log(P->ctx,
P->errorIfBestTransformationNotAvailable ? PJ_LOG_ERROR : PJ_LOG_DEBUG,
msg.c_str());
}

/**************************************************************************************/
PJ_COORD proj_trans (PJ *P, PJ_DIRECTION direction, PJ_COORD coord) {
/***************************************************************************************
Expand Down Expand Up @@ -348,6 +383,12 @@ similarly, but prefers the 2D resp. 3D interfaces if available.
if( res.xyzt.x != HUGE_VAL ) {
return res;
}
else if( P->errorIfBestTransformationNotAvailable ||
P->ctx->warnIfBestTransformationNotAvailable ) {
warnAboutMissingGrid(alt.pj);
if( P->errorIfBestTransformationNotAvailable )
return res;
}
if( iRetry == N_MAX_RETRY ) {
break;
}
Expand Down Expand Up @@ -1894,11 +1935,13 @@ PJ *proj_create_crs_to_crs_from_pj (PJ_CONTEXT *ctx, const PJ *source_crs, cons
if( !ctx ) {
ctx = pj_get_default_ctx();
}
pj_load_ini(ctx); // to set ctx->errorIfBestTransformationNotAvailableDefault

const char* authority = nullptr;
double accuracy = -1;
bool allowBallparkTransformations = true;
bool forceOver = false;
bool errorIfBestTransformationNotAvailable = ctx->errorIfBestTransformationNotAvailableDefault;
for (auto iter = options; iter && iter[0]; ++iter) {
const char *value;
if ((value = getOptionValue(*iter, "AUTHORITY="))) {
Expand All @@ -1915,6 +1958,17 @@ PJ *proj_create_crs_to_crs_from_pj (PJ_CONTEXT *ctx, const PJ *source_crs, cons
"Invalid value for ALLOW_BALLPARK option.");
return nullptr;
}
} else if ((value = getOptionValue(*iter, "ONLY_BEST="))) {
ctx->warnIfBestTransformationNotAvailable = false;
if( ci_equal(value, "yes") )
errorIfBestTransformationNotAvailable = true;
else if( ci_equal(value, "no") )
errorIfBestTransformationNotAvailable = false;
else {
ctx->logger(ctx->logger_app_data, PJ_LOG_ERROR,
"Invalid value for ONLY_BEST option.");
return nullptr;
}
}
else if ((value = getOptionValue(*iter, "FORCE_OVER="))) {
if (ci_equal(value, "yes")) {
Expand Down Expand Up @@ -1963,7 +2017,9 @@ PJ *proj_create_crs_to_crs_from_pj (PJ_CONTEXT *ctx, const PJ *source_crs, cons
ctx, operation_ctx, PROJ_SPATIAL_CRITERION_PARTIAL_INTERSECTION);
proj_operation_factory_context_set_grid_availability_use(
ctx, operation_ctx,
proj_context_is_network_enabled(ctx) ?
(errorIfBestTransformationNotAvailable ||
ctx->warnIfBestTransformationNotAvailable ||
proj_context_is_network_enabled(ctx)) ?
PROJ_GRID_AVAILABILITY_KNOWN_AVAILABLE:
PROJ_GRID_AVAILABILITY_DISCARD_OPERATION_IF_MISSING_GRID);

Expand All @@ -1984,19 +2040,43 @@ PJ *proj_create_crs_to_crs_from_pj (PJ_CONTEXT *ctx, const PJ *source_crs, cons

ctx->forceOver = forceOver;

const int old_debug_level = ctx->debug_level;
if( errorIfBestTransformationNotAvailable || ctx->warnIfBestTransformationNotAvailable )
ctx->debug_level = PJ_LOG_NONE;
PJ* P = proj_list_get(ctx, op_list, 0);
ctx->debug_level = old_debug_level;
assert(P);

if( P != nullptr ) {
P->errorIfBestTransformationNotAvailable = errorIfBestTransformationNotAvailable;
}

if( P == nullptr || op_count == 1 ||
proj_get_type(source_crs) == PJ_TYPE_GEOCENTRIC_CRS ||
proj_get_type(target_crs) == PJ_TYPE_GEOCENTRIC_CRS ) {
proj_list_destroy(op_list);
ctx->forceOver = false;

if( P != nullptr &&
(errorIfBestTransformationNotAvailable ||
ctx->warnIfBestTransformationNotAvailable) &&
!proj_coordoperation_is_instantiable(ctx, P) )
{
warnAboutMissingGrid(P);
if( errorIfBestTransformationNotAvailable ) {
proj_destroy(P);
return nullptr;
}
}

return P;
}

if( errorIfBestTransformationNotAvailable || ctx->warnIfBestTransformationNotAvailable )
ctx->debug_level = PJ_LOG_NONE;
auto preparedOpList = pj_create_prepared_operations(ctx, source_crs, target_crs,
op_list);
ctx->debug_level = old_debug_level;

ctx->forceOver = false;
proj_list_destroy(op_list);
Expand All @@ -2007,6 +2087,10 @@ PJ *proj_create_crs_to_crs_from_pj (PJ_CONTEXT *ctx, const PJ *source_crs, cons
return nullptr;
}

for( auto& op: preparedOpList ) {
op.pj->errorIfBestTransformationNotAvailable = errorIfBestTransformationNotAvailable;
}

// If there's finally juste a single result, return it directly
if( preparedOpList.size() == 1 )
{
Expand Down
22 changes: 21 additions & 1 deletion src/apps/cs2cs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ static const char *oterr = "*\t*"; /* output line for unprojectable input */
static const char *usage =
"%s\nusage: %s [-dDeEfIlrstvwW [args]]\n"
" [[--area name_or_code] | [--bbox west_long,south_lat,east_long,north_lat]]\n"
" [--authority {name}] [--accuracy {accuracy}] [--no-ballpark] [--3d]\n"
" [--authority {name}] [--3d]\n"
" [--accuracy {accuracy}] [--only-best[=yes|=no]] [--no-ballpark]\n"
" [+opt[=arg] ...] [+to +opt[=arg] ...] [file ...]\n";

static double (*informat)(const char *,
Expand Down Expand Up @@ -418,6 +419,8 @@ int main(int argc, char **argv) {
const char* authority = nullptr;
double accuracy = -1;
bool allowBallpark = true;
bool onlyBestSet = false;
bool errorIfBestTransformationNotAvailable = false;
bool promoteTo3D = false;

/* process run line arguments */
Expand Down Expand Up @@ -484,6 +487,15 @@ int main(int argc, char **argv) {
else if (strcmp(*argv, "--no-ballpark") == 0 ) {
allowBallpark = false;
}
else if (strcmp(*argv, "--only-best") == 0 ||
strcmp(*argv, "--only-best=yes") == 0 ) {
onlyBestSet = true;
errorIfBestTransformationNotAvailable = true;
}
else if (strcmp(*argv, "--only-best=no") == 0 ) {
onlyBestSet = true;
errorIfBestTransformationNotAvailable = false;
}
else if (strcmp(*argv, "--3d") == 0 ) {
promoteTo3D = true;
}
Expand Down Expand Up @@ -893,6 +905,14 @@ int main(int argc, char **argv) {
if( !allowBallpark ) {
options.push_back("ALLOW_BALLPARK=NO");
}
if( onlyBestSet ) {
if( errorIfBestTransformationNotAvailable ) {
options.push_back("ONLY_BEST=YES");
}
else {
options.push_back("ONLY_BEST=NO");
}
}
options.push_back(nullptr);
transformation = proj_create_crs_to_crs_from_pj(nullptr, src, dst,
pj_area, options.data());
Expand Down
2 changes: 2 additions & 0 deletions src/ctx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ pj_ctx::pj_ctx(const pj_ctx& other) :
lastFullErrorMessage(std::string()),
last_errno(0),
debug_level(other.debug_level),
warnIfBestTransformationNotAvailable(other.warnIfBestTransformationNotAvailable),
errorIfBestTransformationNotAvailableDefault(other.errorIfBestTransformationNotAvailableDefault),
logger(other.logger),
logger_app_data(other.logger_app_data),
cpp_context(other.cpp_context ? other.cpp_context->clone(this) : nullptr),
Expand Down
16 changes: 16 additions & 0 deletions src/filemanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1819,6 +1819,16 @@ void pj_load_ini(PJ_CONTEXT *ctx) {
ctx->ca_bundle_path = ca_bundle_path;
}

// Load default value for errorIfBestTransformationNotAvailableDefault
// from environment first
const char *proj_only_best_default = getenv("PROJ_ONLY_BEST_DEFAULT");
if (proj_only_best_default && proj_only_best_default[0] != '\0') {
ctx->errorIfBestTransformationNotAvailableDefault =
ci_equal(proj_only_best_default, "ON") ||
ci_equal(proj_only_best_default, "YES") ||
ci_equal(proj_only_best_default, "TRUE");
}

ctx->iniFileLoaded = true;
auto file = std::unique_ptr<NS_PROJ::File>(
reinterpret_cast<NS_PROJ::File *>(pj_open_lib_internal(
Expand Down Expand Up @@ -1878,6 +1888,12 @@ void pj_load_ini(PJ_CONTEXT *ctx) {
}
} else if (ca_bundle_path == nullptr && key == "ca_bundle_path") {
ctx->ca_bundle_path = value;
} else if (proj_only_best_default == nullptr &&
key == "only_best_default") {
ctx->errorIfBestTransformationNotAvailableDefault =
ci_equal(value, "ON") ||
ci_equal(value, "YES") ||
ci_equal(value, "TRUE");
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/iso19111/c_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -521,10 +521,13 @@ PJ *proj_clone(PJ_CONTEXT *ctx, const PJ *obj) {
if (newPj) {
newPj->descr = "Set of coordinate operations";
newPj->ctx = ctx;
const int old_debug_level = ctx->debug_level;
ctx->debug_level = PJ_LOG_NONE;
for (const auto &altOp : obj->alternativeCoordinateOperations) {
newPj->alternativeCoordinateOperations.emplace_back(
PJCoordOperation(ctx, altOp));
}
ctx->debug_level = old_debug_level;
}
return newPj;
}
Expand Down
3 changes: 3 additions & 0 deletions src/proj_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,7 @@ struct PJconsts {
**************************************************************************************/
std::vector<PJCoordOperation> alternativeCoordinateOperations{};
int iCurCoordOp = -1;
bool errorIfBestTransformationNotAvailable = false;

/*************************************************************************************

Expand Down Expand Up @@ -694,6 +695,8 @@ struct pj_ctx{
std::string lastFullErrorMessage{}; // used by proj_context_errno_string
int last_errno = 0;
int debug_level = PJ_LOG_ERROR;
bool warnIfBestTransformationNotAvailable = true; /* to remove in PROJ 10? */
bool errorIfBestTransformationNotAvailableDefault = false;
void (*logger)(void *, int, const char *) = nullptr;
void *logger_app_data = nullptr;
struct projCppContext* cpp_context = nullptr; /* internal context for C++ code */
Expand Down
Loading