Cannot set Distance constraint to horizontal or vertical for lines [BUG]

[TimStrauven]

Contact Details

No response

Description

When adding a distance constraint on a line, it will be placed along the parallel of that line. In the options of the constraint it displays horizontal and vertical, but clicking it results in an error in "model/distance.py" line 72. This line is: distance = _get_aligned_distance(self.entity1, self.entity2, alignment) While on a line distance constraint entity2 is "None". This then triggers an error in _get_aligned_distance which is clearly created to deal with points:

def _get_aligned_distance(p_1, p_2, alignment):
    if alignment == "HORIZONTAL":
        return abs(p_2.co.x - p_1.co.x)
    if alignment == "VERTICAL":
        return abs(p_2.co.y - p_1.co.y)
    return (p_2.co - p_1.co).length

As it was designed for points in the first place my suggested solution would be to never place distance constraints on lines, but always detect the endpoints and always use these points instead. This can be done in "operators/add_distance.py" in the main method by checking if self.entity1 is a 2D line and self.entity2 is None. Then getting the endpoints of the line and feeding those into the add_distance function.

Addon Version

0.27.3

Blender Version

4.1

What platform are you running on?

Linux

[TimStrauven]

It took some effort to set entity1 and 2 since there are not setters directly, however after figuring out how the stateful operators were implemented I could set them this way. This solution works well, also with the other proposed change to check which constraint exists.

The changes are as explained above in "operators/add_distance.py" extra imports:

from ..model.line_2d import SlvsLine2D
from ..model.point_2d import SlvsPoint2D

and updated main:

    def main(self, context):
        if type(self.entity1) == SlvsLine2D and self.entity2 == None:
            dependencies = self.entity1.dependencies()
            if type(dependencies[0]) == SlvsPoint2D and type(dependencies[1]) == SlvsPoint2D:
                for i in range(0,2):
                    self.state_index = i # set index to access i state_data
                    self.state_data["hovered"] = -1
                    self.state_data["type"] = type(dependencies[i])
                    self.state_data["is_existing_entity"] = True
                    self.state_data["entity_index"] = dependencies[i].slvs_index      

        if not self.exists(context, SlvsDistance):
            self.target = context.scene.sketcher.constraints.add_distance(
                self.entity1,
                self.entity2,
                sketch=self.sketch,
                init=not self.initialized,
                **self.get_settings(),
            )
        return super().main(context)

If you are ok with this and the other one I'll create a PR for both at once.

[hlorus]

That looks nice, i think it makes sense to always use points with the constraint and just get them implicitly when the user selects a line.

I don't really see the reason why we have to change stuff in the stateful operator, isn't it enough to just retrieve the points and supply them in the call to "self.target = context.scene.sketcher.constraints.add_distance()"?

[hlorus]

Btw this will also need some versioning, see versioning.py

[TimStrauven]

Yes, my first easy approach was indeed just passing in the points directly, and this works fine. However, the exists function depends on self.entity1 and 2, as does every other flow of constraints within the project. By using this state_data change it changes the placeholders for self.entity1 and self.entity2 directly. So for anyone that needs to add something to any constraint later, the workflow with entity1 and 2 will stay the same and won't be obscured in a weird way. Thats why I took the extra effort to implement it this way.

For versioning, do you mean bumping cad_sketcher version or do you mean triggering recalc_pointers?

[hlorus]

I would try to keep it simple here. The states sort of represent the user input. Changing stuff there might be confusing to the user.

Maybe just add a variable to exists() to overwrite the default list of operator entities?

If you'd really want to change the operator's state properties we should at least move the logic to the stateful operator by adding some kind of concept, like an "implicit selection" or similar. Just be aware that this would be a bigger task on its own.

[TimStrauven]

I would try to keep it simple here. The states sort of represent the user input. Changing stuff there might be confusing to the user.

In this case changing it is what we are trying to achieve. I don't see how it would be confusing to a user, they won't even notice it. What would be the difference for the user this way, or by sending the points directly into "context.scene.sketcher.constraints.add_distance"?? If anything, changing the states makes it less confusing for a dev that needs to change/add something later, as I explained above.

Maybe just add a variable toexists() to overwrite the default list of operator entities?

See my previous comment, this obscures the values of self.entity1 and 2 and should be avoided because of their use everywhere.

If you'd really want to change the operator's state properties we should at least move the logic to the stateful operator by adding some kind of concept, like an "implicit selection" or similar. Just be aware that this would be a bigger task on its own.

While I do agree, I'm not sure if I'm willing to take this task. And if it is even worth the effort because it has a high probability of introducing other bugs for a single use case that is already solved in the code above.

As mentioned in my other comment, maybe talking over Discord would be easier.