-
Notifications
You must be signed in to change notification settings - Fork 764
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
Changes from 5 commits
fdd9901
d11c9d5
27b2fa4
30b2e97
ec0ce09
4fb15a2
62d2291
b1db3f6
9724764
97157e3
9020ae2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 However, your concerns are valid. If you want to do it once per There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
/*************************************************************************************** | ||
|
@@ -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; | ||
} | ||
|
@@ -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="))) { | ||
|
@@ -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")) { | ||
|
@@ -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); | ||
|
||
|
@@ -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); | ||
|
@@ -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 ) | ||
{ | ||
|
There was a problem hiding this comment.
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 intoproj_create_crs_to_crs_from_pj
, correct?There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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 perhapsCoordinateOperationFactory::Private::createOperationsCompoundToGeog()
:-) )There was a problem hiding this comment.
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