Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve auto constraints #425

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Improve auto constraints #425

wants to merge 6 commits into from

Conversation

bonjorno7
Copy link
Contributor

By using a context manager which prevents you from adding constraints that stop the sketch from solving.

@bonjorno7
Add context manager for safe constraints
9dffe59 
You add the constraints as normal, and when the context manager exits it checks if the sketch still solves.
If it doesn't, it removes the last constraint and checks again.
This continues until either the sketch solves, or all the constraints that were added inside the context manager are removed.
The fact that the loop checks whether the sketch solves one more time after the list of new constraints is empty is deliberate; it makes sure the sketch is up to date.
The loop doesn't check whether the list is empty, because when it's empty the sketch should solve (hence the check to make sure it solved before any of the constraints were added).
@bonjorno7
Use safe_constraints for add_line_2d
aa1f283 
This operator was the simplest to add it to, and serves as an example of how to use the context manager.
@bonjorno7 bonjorno7 mentioned this pull request Nov 19, 2023
Open
@bonjorno7
Copy link
Contributor Author

The linter seems to dislike long lines.
I didn't think they would be an issue, because the project has lots of them already.
I suppose I can split things across more lines if necessary.

@hlorus
Copy link
Owner

hlorus commented Nov 20, 2023

Great stuff! I see one major problem tho, since constraints.all is not sorted by creation date the contextmanager might remove constraints that weren't added in its context which is bad IMO.
Maybe we could record a list of added constraints or just check how many have been added per type.

Furthermore checking if the system solves shouldn't change entities. Maybe we can add an option to the solver which doesn't apply changes.

hlorus
hlorus reviewed Nov 20, 2023
View reviewed changes
operators/utilities.py Outdated
sketch = sketch or context.scene.sketcher.active_sketch
constraints = constraints or context.scene.sketcher.constraints

solve = solve_system(context, sketch)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably avoid the initial solve check and just get the value from sketch.solver_state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I wasn't aware of that, I'll use it instead of calling the function then yeah.

@hlorus
Copy link
Owner

hlorus commented Nov 20, 2023

The linter seems to dislike long lines.
I didn't think they would be an issue, because the project has lots of them already.
I suppose I can split things across more lines if necessary.

Don't worry about the linter too much, it's more of an aid.

@bonjorno7
Copy link
Contributor Author

Great stuff! I see one major problem tho, since constraints.all is not sorted by creation date the contextmanager might remove constraints that weren't added in its context which is bad IMO. Maybe we could record a list of added constraints or just check how many have been added per type.

I assumed that it'd be sorted, my bad. How would you go about getting a list that is sorted?
Alternatively I can use a set and subtract them to get the new constraints; though they would be in arbitrary order.

Furthermore checking if the system solves shouldn't change entities. Maybe we can add an option to the solver which doesn't apply changes.

I'm not sure what you mean by this (probably because I don't know much about this code base).

@bonjorno7
Don't rely on order for safe_constraints
e1063b2 
Instead of removing the newest constraint until the sketch solves, it just removes all of the new constraints if the sketch fails to solve.
It calls solve again at the end to make sure it's updated.
@bonjorno7
Copy link
Contributor Author

It now only removes constraints that are actually new.
If the sketch fails, it removes all of the new constraints.
Ideally it'd find out which constraints specifically are conflicting and remove those, but I don't know how to do that at this time.

@hlorus
Copy link
Owner

hlorus commented Nov 22, 2023

It now only removes constraints that are actually new.
If the sketch fails, it removes all of the new constraints.
Ideally it'd find out which constraints specifically are conflicting and remove those, but I don't know how to do that at this time.

I think that's a fair tradeoff and probably easy for the user to understand as the tool either creates constraints or not.
Just to note: the solver reports which constraints are failing, you can check that through constraint.failed. We could potentially try to first remove the failed, newly-created constraints however i'm not sure how consistent that behavior would be.

@hlorus
Copy link
Owner

hlorus commented Nov 22, 2023

Furthermore checking if the system solves shouldn't change entities. Maybe we can add an option to the solver which doesn't apply changes.

I'm not sure what you mean by this (probably because I don't know much about this code base).

The solver simply changes the position of geometric entities based on the constraints. We don't want that to happen always, usually only from the fini method of operators. With this PR the solver is triggered from the "main" method of the line_2d operator which runs every frame and causes the first point of the line to follow the vertical/horizontal linewhich is bad.
I think it should be enough to add a new option to the solve method in solver.py, something like update_entities=True or similar.

@bonjorno7
Add update_entities option for Solver.solve
f0dc384 
Also added the report and update_entities arguments to solve_system and SlvsSketch.solve, so they're available regardless of how you call solve.
@bonjorno7
Remove only failed constraints in safe_constraints
5f07fba 
There's still two solve calls, but now they don't update entities at least.
The first call is to see if any constraints failed, the second to update the UI after removing those constraints (otherwise it says "Inconsistent" during and after the operation).
@bonjorno7
Copy link
Contributor Author

The solver simply changes the position of geometric entities based on the constraints. We don't want that to happen always, usually only from the fini method of operators. With this PR the solver is triggered from the "main" method of the line_2d operator which runs every frame and causes the first point of the line to follow the vertical/horizontal linewhich is bad. I think it should be enough to add a new option to the solve method in solver.py, something like update_entities=True or similar.

Ah alright, makes sense.
I've added the update_entities option (True by default), so safe_constraints no longer updates entities.

I think that's a fair tradeoff and probably easy for the user to understand as the tool either creates constraints or not.
Just to note: the solver reports which constraints are failing, you can check that through constraint.failed. We could potentially try to first remove the failed, newly-created constraints however i'm not sure how consistent that behavior would be.

I made it remove only the constraints where failed=True; though it might make sense to make this an option.
I feel like if you're adding multiple constraints inside the same safe_constraints block, it's probably because they rely on each other.
So it might make more sense to remove all of the constraints in that case, rather than only the failed ones.

@bonjorno7
Add remove_only_failed option to safe_constraints
1c74d60 
False by default, meaning it will remove all new constraints if any of them failed.
If set to True, it will only remove new constraints that failed, and keep successful new constraints.
@bonjorno7
Copy link
Contributor Author

I made it remove only the constraints where failed=True; though it might make sense to make this an option.
I feel like if you're adding multiple constraints inside the same safe_constraints block, it's probably because they rely on each other.
So it might make more sense to remove all of the constraints in that case, rather than only the failed ones.

Added the option, so by default it removes all new constraints if any of them failed; if enabled it only removes failed constraints and keeps the successful ones.

@bonjorno7
Copy link
Contributor Author

Oh and would it be possible to set the linter to 120 line length?
That'd be consistent with the actual code base, so it would stop failing every time.

@hlorus
Copy link
Owner

hlorus commented Nov 24, 2023

Looks good so far. Do you plan to change the other tools that add constraints as well with this PR?

@bonjorno7
Copy link
Contributor Author

If you think the context manager is good now, I can start adding the other operators yeah.

@hlorus
Copy link
Owner

hlorus commented Nov 24, 2023

If you think the context manager is good now, I can start adding the other operators yeah.

It looks good to me so far👍🏼

@bonjorno7
Copy link
Contributor Author

I could use some help coming up with cases where auto constraints fail actually.
It's easy enough for line_2d, but other than that I'm not sure.

@hlorus
Copy link
Owner

hlorus commented Nov 26, 2023

I think it's mainly a problem with some advanced tool like bevel/trim. If there are no cases where it fails with the simple tools we can just leave them as they are.

@bonjorno7
Copy link
Contributor Author

I forgot there are other non-okay states that are not inconsistent, so the code doesn't work as it should in all cases.
I'm working on fixing that at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlorus hlorus hlorus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bonjorno7 @hlorus