Cannot set Distance constraint to horizontal or vertical for lines [BUG] about cad_sketcher HOT 6 OPEN

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.

from cad_sketcher.

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()"?

from cad_sketcher.

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

from cad_sketcher.

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?

from cad_sketcher.

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.

from cad_sketcher.

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.

from cad_sketcher.