CAD_Sketcher icon indicating copy to clipboard operation
CAD_Sketcher copied to clipboard

Published 20 hours ago • verified publisher icon hlorus

Add missing solve return values

Open abergmeier opened this issue 2 years ago • 6 comments

According to C headers there are values 4 and 5 possible.

abergmeier avatar Aug 10 '22 19:08 abergmeier

This currently is missing two things:

  1. it does not force re-solving. Maybe this should be done for every new release/code anyway?
  2. It does not show which constraints are redundant.

abergmeier avatar Aug 10 '22 20:08 abergmeier

Nice fix! I think the documentation was wrong as the 5th error was never explained. Now that all the possible errors are implemented the "Solver returned unknown error" could probably be removed.

hlorus avatar Aug 10 '22 20:08 hlorus

This currently is missing two things:

  1. it does not force re-solving. Maybe this should be done for every new release/code anyway?
  2. It does not show which constraints are redundant.

  1. I don't really get what you mean here.

  2. Afaik solvespace doesn't have support to report which exact constraints are redundant/overconstrained, same with degree of freedom.

hlorus avatar Aug 10 '22 20:08 hlorus

I don't really get what you mean here.

Since previously the code did clamp e.g. 5 to 4 if you open a blender file with the new code it shows thus the error message for 4 - which is actually wrong (the value should now be 5). Maybe a good idea to force reevaluating of all cached state when loading a new version of the AddOn.

abergmeier avatar Aug 10 '22 20:08 abergmeier

Afaik solvespace doesn't have support to report which exact constraints are redundant/overconstrained, same with degree of freedom.

I am not sure. Looking at the C code where they do further redundant handling it seems like they do some more processing. Will try to look into this the next days some more.

abergmeier avatar Aug 10 '22 20:08 abergmeier

Ahh i see, IMO this is really just a minor issue which isn't a big deal in pre 1.0 versions. Otherwis it could be adjusted in versioning.py.

hlorus avatar Aug 10 '22 20:08 hlorus

So since SolveSpaceUI::SolveGroupAndReport runs solving twice when it returns redundancies - maybe we should adopt the same mechanism?

abergmeier avatar Aug 19 '22 17:08 abergmeier

Just re-try solving again whenever the solver fails or just with a specific return value of the solver?

hlorus avatar Aug 23 '22 11:08 hlorus

@abergmeier This PR is IMO in a good state but is still marked as WIP... I'd say not re-solving old files when opening isn't a big deal as we haven't reached 1.0 yet and we also have a manual update button. Additional changes like solving twice for redundant error is also better kept in a separate PR.

hlorus avatar Aug 30 '22 06:08 hlorus

Seems to work brilliantly, except perhaps for the "redundant" + Ok warning message.

image

Maybe a good idea to force reevaluating of all cached state when loading a new version of the AddOn.

Great point btw, perhaps we should consider this before merging to master? Perhaps we can make a test/develop branch and create a new PR for the reevaluation-on-update feature?

amrsoll avatar Sep 07 '22 08:09 amrsoll

This seems to work great, just some minor notes:

  • It's easy to overlook the text change to "Redundant OK", maybe we could use a different icon if there's just a note, maybe the "INFO" icon?
  • The constraints that are reported by the solver will be marked with "Failed" which might be a bit misleading.

hlorus avatar Sep 13 '22 09:09 hlorus

Seems to work brilliantly, except perhaps for the "redundant" + Ok warning message.

I would also just go with "Redundant" and the info icon.

hlorus avatar Sep 13 '22 13:09 hlorus

The constraints that are reported by the solver will be marked with "Failed" which might be a bit misleading.

I think it is OK given there is no other way to know which constraints are redundant atm.

amrsoll avatar Sep 13 '22 19:09 amrsoll