-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Do not allow invalid map settings #2044
Conversation
Fix qgis#12757 Throws QgsException on uninvertible matrix
With this implementation I do get a popup (rather than an assertion failure) when going scale-wild. |
NOTE: the change involves no signature changes for easier backport to 2.8 |
Instead of: "Unusable (not-invertible) matrix" |
"anymore" ? Was it really possible before ?
I've been having many issues with rendering at high zoom levels
(while trying to see subtle shifts between features), I guess they
are due to similar numerical stability issues. Is it preferred to
do arbitrary things rather than throwing an exception ? It could be
another option...
I'd love to see a real usecase in which that error is triggered.
Btw, before merging this, earlier catches should likely be added,
probably inside MapSettings as I think right now MapSettings get
out of sync with its MapToPixel member.
|
@rduivenvoorde : certainly a better phrasing is necessary, invertible matrices are going to scare off people :) |
Could we have the implementation without the use of exceptions? Exceptions are used in QGIS code very sparsely and not expected to be thrown by ordinary code. Introducing them in a class like QgsMapToPixel may require addition of extra wrapping code everywhere where QgsMapToPixel is used. I think the best would be to change signature of the setters to return bool instead of void. |
Agree on changing signature, but as I mentioned I hadn't done so to
allow for the fix to get into the 2.8 branch.
But maybe for 2.8 it is even better to just let the map rendering
go wild by simply removing the assert...
|
If we don't want to change the signature for 2.8, what about having one commit that will do the fix and keep the signature (will be ported to 2.8) and a followup commit that will introduce return of bool (will not be ported to 2.8) ... |
@wonder-sk @strk isn't changing the return type from void -> bool allowed and not classified as a break? It's been treated that way in the past. I don't see an issue with allowing that change into 2.8. |
@nyalldawson yes... I think @strk mainly wanted to leave the changes in 2.8 as minimal as possible |
I'm not aware of the current policy.
Changing a return code might be "ok" for the API,
but most likely not for the ABI.
|
We do not really keep ABI compatibility between different versions - so it would be also fine to change the signature in 2.8 if the API does not get broken (not the case here) |
In any case it'll still take handling of the error from all the callers,
which would then require changing methods like
QgsMapToPixel::setMapUnitsPerPixel to also return bool rather than void,
so QgsMapSettings::updateDerived() so QgsMapSettings::setExtent() and
so QgsMapCanvas::setExtent() ...
are you sure you don't prefer an exception to be cought directly
from QgsMapCanvas (for example) ?
|
@strk @wonder-sk @nyalldawson @jef-n would be great if this can get fixed. Can we choose one of the fixes before 2.10 ? |
To reanimate this PR: I think a return value is preferable to an exception. |
@m-kuhn if RAII was used, the risk of leak with uncaught exception would be very small. I prefer code that fails noisily. But in the current state of QGIS... grep -rn 'delete '|wc -l
2239 ... what you say makes sense. |
... so why not the other way around ? fix exception safety and use exceptions ? |
was that an offer? :) |
Cherry-picked manually and de-exception-ified in 1bf9844 Thanks Sandro et al |
Fix #12757
Throws QgsException on uninvertible matrix