[BUG] CAD Sketcher allows us to apply the hor/vert constraints multiple times to an element about cad_sketcher HOT 14 OPEN

Hi hlorus,

Since I already found the solution for the other constraint issue i took a look at this one as well.

The reason here is in the file "operators/add_geometric_constraints.py", the main function of "VIEW3D_OT_slvs_add_horizontal" does indeed not check if the constraint already exists.

I solved it as follows, with two extra imports on the file:

  from ..model.horizontal import SlvsHorizontal
  from ..model.line_2d import SlvsLine2D
  
  class VIEW3D_OT_slvs_add_horizontal(Operator, GenericConstraintOp):
      """Add a horizontal constraint"""
      
      bl_idname = Operators.AddHorizontal
      bl_label = "Horizontal"
      bl_options = {"UNDO", "REGISTER"}
  
      type = "HORIZONTAL"
  
      def main(self, context):    
          # check if the constraint already existed on the selected line or points
          hor_constr_exists = False
          for c in context.scene.sketcher.constraints.all:
              if self.entity1 in c.dependencies() and type(c) == SlvsHorizontal:
                  if type(self.entity1) == SlvsLine2D:
                      hor_constr_exists = True
                  elif self.entity2 in c.dependencies():
                      hor_constr_exists = True
  
          if not hor_constr_exists:
              self.target = context.scene.sketcher.constraints.add_horizontal(
                      self.entity1,
                      entity2=self.entity2,
                      sketch=self.sketch,
                  )
          
          return super().main(context)

I was still looking to get the constraints from the active sketch only instead of all to save some items from the loop, but could not find it.

It works as intended for all tests i could come up with, please let me know any comments or thoughts.
If everything looks ok, I'll do the same for vertical constraints and create a PR.

Regards,
T

from cad_sketcher.

This problem is likely not specific to horizontal/vertical constraints but rather applies to all constraint tools and probably also to tools that add entities. Therefore a possible solution should try to be somewhat generic, otherwise we end up reinventing the wheel for every tool and have a lot of duplicated code.

from cad_sketcher.

Indeed, I assumed it was only happening on horizontal/vertical constraints.
I took a further look at it, but could not come up with an idea that solves it immediately for all entities in general.

A solution could be something like this in the file "solver.py" you could add a call in the "solve" function to this function (without the printouts):

 def _remove_duplicate_constraints(self, context):
        constraints_with_deps = []
        for c in context.scene.sketcher.constraints.all:
            duplicate_found = False
            for constraint_with_deps in constraints_with_deps:
                if type(c) == constraint_with_deps["constraint_type"]:
                    if set(c.dependencies()) == set(constraint_with_deps["constraint_dependencies"]):
                        duplicate_found = True
                        break

            if duplicate_found:
                print("duplicate constraint found!")
                print(type(c))
                print(f"    {c.dependencies()}")
                print("#######################################")
                context.scene.sketcher.constraints.remove(c)
                self.solve()
            else:
                constraint_with_deps = {}
                constraint_with_deps["constraint_type"] = type(c)
                constraint_with_deps["constraint_dependencies"] = c.dependencies()
                constraints_with_deps.append(constraint_with_deps)

This seems to solve it for all constraints. I had to add a call back to the solve method in case there is more than one duplicate, this way it clears all duplicates at once.

For other duplicate entities we could add a separate function looking at lines, arcs etc...
Does this sound like a decent solution?

Update:
The above works when there are duplicate constraints and deletes duplicates while creating/deleting entities.
However I get errors pointing to dangling constraints while dragging existing lines around in this situation.

File "/gizmos/base.py", line 54, in draw_select
if not constr.visible:
AttributeError: 'NoneType' object has no attribute 'visible'

Is "context.scene.sketcher.constraints.remove(c)" the correct way to delete the constraint?

from cad_sketcher.

I would avoid deleting constraints from the solver, this isn't really it's responsibility and could cause issues when we try to delete multiple constraints at once, see: https://docs.blender.org/api/current/info_gotcha.html#no-updates-after-setting-values

Imo the better approach is to avoid creating duplicates in the first place as you've tried first. Maybe just turn the "hor_constr_exists" lookup into a utility function that works for other constraint types as well?

from cad_sketcher.

My focus got a bit too much into trying to clean up existing duplicates instead of just preventing them.
Following your advice I added the following to "class GenericConstraintOp(Operator2d):" in "operators/base_constraint.py"

    def exists(self, context, constraint_type=None) -> bool:
        new_dependencies = [i for i in [self.entity1, self.entity2, self.sketch] if i is not None]
        for c in context.scene.sketcher.constraints.all:
            if type(c) == constraint_type:             
                if set(c.dependencies()) == set(new_dependencies):
                    print("constraint already exists!")
                    return True
        return False

Then the main method of every class that creates a constraint gets an extra import for the type and a check to the above function. So for all classes in "operators/add_geometric_constraints.py" and also for "operators/add_distance.py", "operators/add_diameter.py" and "operators/add_angle.py".

Example for horizontal:

from ..model.horizontal import SlvsHorizontal

class VIEW3D_OT_slvs_add_horizontal(Operator, GenericConstraintOp):
    """Add a horizontal constraint"""

    bl_idname = Operators.AddHorizontal
    bl_label = "Horizontal"
    bl_options = {"UNDO", "REGISTER"}

    type = "HORIZONTAL"

    def main(self, context):    
        if not self.exists(context, SlvsHorizontal):
            self.target = context.scene.sketcher.constraints.add_horizontal(
                    self.entity1,
                    entity2=self.entity2,
                    sketch=self.sketch,
                )
        return super().main(context)

This works perfectly for preventing the issue on all tests I did, does it look good to you?
Thanks again for pointing out the Blender specifics, still have much to learn about those.

from cad_sketcher.

I'd say that looks perfect! Would be great to see a PR :)

from cad_sketcher.

Not completely, just found an edge case: If you put a distance on 2 points you should be able to have both a horizontal and vertical distance constraint, this code doesn't allow that. So I'm going to iron that out first.
Let me know if you can think of other edge cases like this.

On that note I found another bug/missing feature, you can't set dimensions to horizontal or vertical when its a line.

from cad_sketcher.

If I update the "exists" function this way I think everything is covered. Let me know if I can create a PR this way.

    def exists(self, context, constraint_type=None) -> bool:
        new_dependencies = [i for i in [self.entity1, self.entity2, self.sketch] if i is not None]

        distance_dof = 0
        has_3D_entity = ((SlvsPoint3D == type(self.entity1) or SlvsPoint3D == type(self.entity2)) or
                        (SlvsLine3D == type(self.entity1) or SlvsLine3D == type(self.entity2)))

        if has_3D_entity:
            max_distance_dof = 3
        else:
            max_distance_dof = 2
        
        if (type(self.entity1) == SlvsCircle or type(self.entity1) == SlvsArc) and type(self.entity2) == SlvsLine2D:
            max_distance_dof = 1

        for c in context.scene.sketcher.constraints.all:
            if type(c) == constraint_type:
                if set(c.dependencies()) == set(new_dependencies):
                    if constraint_type == SlvsDistance:
                            distance_dof +=1
                    else:
                        return True
        
        if distance_dof < max_distance_dof:
            return False
        else:
            return True

I was searching for the status of 3D entities, and their constraints, but currently it only seems half implemented? This code should also cover future plans with it.

Since I went quite far researching this I think I also have the solution that you cannot set line distance constraints to vertical/horizontal: on creation if it is a line you can get the endpoints and set entity1 and 2 to those instead. Then you'll never have duplicates of dimensions on the endpoints + the line, and in case you delete the line the dimension will not be deleted along with it but stay on the points. It would also be one click less for the user (select 1 line vs 2x point).
I'll create a new bug report with the solution for this (have not yet tested it though).

from cad_sketcher.

I don't exactly get the approach with the DoF, why would that matter? 3d constraints aren't supported, there would have to be a bigger effort to get it, therefore i wouldn't look too much into that.

Can't we just check if the constraints settings are the same in the exist method? Afaik constraints keep a list of their main settings in constraint.props, maybe we can use that? Or just hardcode it in the operator.

from cad_sketcher.

The dof (maybe this name is a bit confusing) is just to check how many degrees of freedom are taken by a distance constraint. Following the matrix in model/distance.py I checked how many dimensions each type could have and modified the exists method accordingly (3D was just added in case it would be implemented at some point in the future):

    #   e1       e2
    #   ----------------
    #   line     [none]    -> max 2 dimensions (hor/vert available)
    #   point    point    -> max 2 dimensions (hor/vert available)
    #   point    line    -> max 1 dimension (forgot to add a check for this one in code above defaulting to max 1 and checking for max 2 would be better)
    #   arc      point    -> max 1 dimension
    #   arc      line    -> max 1 dimension
    #   circle   point    -> max 1 dimension
    #   circle   line    -> max 1 dimension

Using the constraint.props as it is now would not be sufficient, since they only seem to hold the value.
e.g. a distance between two points at 45degrees would have the same value for a horizontal and vertical dimension, thus incorrectly triggering that it already exists.

Maybe we can add a new property there, but I don't see the advantage since we would still need some way to count them (aka the dof way I used).

from cad_sketcher.

Updated for the one I forgot and added a check for entity2 since this doesn't exits on a Diameter/Radius it would look like this:

    def exists(self, context, constraint_type=None) -> bool:
        if hasattr(self, "entity2"):
            new_dependencies = [i for i in [self.entity1, self.entity2, self.sketch] if i is not None]

            if ((SlvsPoint3D == type(self.entity1) or SlvsPoint3D == type(self.entity2)) or
                (SlvsLine3D == type(self.entity1) or SlvsLine3D == type(self.entity2))):
                max_distance_dof = 3            
            elif ((type(self.entity1) == SlvsLine2D and self.entity2 == None) or 
                type(self.entity1) == SlvsPoint2D and type(self.entity2) == SlvsPoint2D):
                max_distance_dof = 2
            else:
                max_distance_dof = 1
        else:
            new_dependencies = [i for i in [self.entity1, self.sketch] if i is not None]
            max_distance_dof = 1

        
        distance_dof = 0
        for c in context.scene.sketcher.constraints.all:
            if type(c) == constraint_type:
                if set(c.dependencies()) == set(new_dependencies):
                    if constraint_type == SlvsDistance:
                            distance_dof +=1
                    else:
                        return True
        
        if distance_dof < max_distance_dof:
            return False
        else:
            return True

from cad_sketcher.

So you're trying to determine if another constraint of the same type makes sense based on the number of DoFs they constrain together with the intention to avoid overconstraining, am i understanding correctly?
Afaik we can't avoid overconstraining anyway as it also depends on other constraints, right?
I would prefer to just avoid duplicated constraints on a data level, where the operator checks if there's already a constraint with the same entities and properties.
Maybe we can add a list of property names to each constraint that should be respected for comparison.

from cad_sketcher.

If we'd like to prevent overconstraining i'd advise to just run the solver in the fini() method and then abort/undo the operation.

Also note that some constraints can have up to 4 entities. So the exists() method should check if all of them overlap with an existing constraint.

from cad_sketcher.

So you're trying to determine if another constraint of the same type makes sense based on the number of DoFs they constrain together with the intention to avoid overconstraining, am i understanding correctly?

No, the intention has nothing to do with overconstraining.
It has the intention to avoid duplicates based on when it would be a duplicate, defined by its type (the matrix explained previously).

Afaik we can't avoid overconstraining anyway as it also depends on other constraints, right?

Right.

I would prefer to just avoid duplicated constraints on a data level, where the operator checks if there's already a constraint with the same entities and properties.

This is exactly what the code above does...

Maybe we can add a list of property names to each constraint that should be respected for comparison.

I don't see the need as the rules are already defined by its type (matrix), which the code above uses.
The only prop that could be added is what I called above "max_distance_dof" (naming can be changed). The logic in the exists method would then be spread out over all of the constraint operators (for some it would be just adding the property, but for others like distance it would require extra logic).
On top of that the exists method would still need to do an individual hasattr check on self.entity2 (because diameter/radius doesn't have it) or we go to a similar situation as in bug report 459 about overriding entity1 and 2.
Personally I think it is much more clear to have all this logic centralised in the exists method.

If we'd like to prevent overconstraining i'd advise to just run the solver in the fini() method and then abort/undo the operation.

No, the solver itself makes mistakes;
Try adding a circle and a line, give them a tangent+coincident constraint without anything else and it will tell you it is overconstrained. So this is not a good option as long as the solver is not 100%.

Even if the solver would be perfect it would still result in a strange user experience (you might want to overconstrain it intentionally while drawing, to see which other constraint you can remove to improve the design. This is how most major CAD packages behave).

Also note that some constraints can have up to 4 entities. So the exists() method should check if all of them overlap with an existing constraint.

Checking overconstrained is not the intention. Not sure if that statement about 4 entities is correct, or if my interpretation is wrong (please elaborate how or which constraint has 4 entities, maybe you meant an entity can have 4 constraints?).

Conclusion:
I'm not trying to sound like a know-it-all but for me the solution is there. Maybe take another look at it, test, discard or change it.
I honestly don't know how or if I can further improve it at the moment. (Apart from maybe adding that one property everywhere. But I think this just makes it less clear.)

I joined the CAD Sketcher Discord (TimS), we can also talk there if that makes things easier.

from cad_sketcher.