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.

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

simplex_solver::change_weight() and simplex_solver::change_strength() don't work #33

Open
ghost opened this issue Dec 12, 2014 · 16 comments

Comments

@ghost
Copy link

ghost commented Dec 12, 2014

I looked into these as a more stable way of changing weights and strengths than just removing and adding the constraint again. Sadly, this method seems to have no effect and looking into unit_tests.cpp it is untested too.

Nocte- added a commit that referenced this issue Dec 15, 2014
It was mentioned in #33 that these functions were untested.
@Nocte-
Copy link
Owner

Nocte- commented Dec 15, 2014

I've added tests for those two functions, and they seem to work. Could you post a snippet of your code?

@hfossli
Copy link
Collaborator

hfossli commented Dec 15, 2014

👍

@ghost
Copy link
Author

ghost commented Dec 17, 2014

@Nocte- Hmmm, this test doesn't work for me:

BOOST_AUTO_TEST_CASE(change_strenght)
{
    auto v = variable {0};
    auto c1 = constraint { v == 42 };
    auto c2 = constraint { v == 21, strength::medium() };
    auto solver = simplex_solver {};

    solver.add_constraints({ c1, c2 });
    BOOST_CHECK_EQUAL(v.value(), 42);

    solver.change_strength(c1, strength::weak());
    BOOST_CHECK_EQUAL(v.value(), 21);
}

It does pas though if I initialize c1 with a non-required strength... any ideas?

@Nocte-
Copy link
Owner

Nocte- commented Dec 18, 2014

That would make sense. Required constraints don't have error variables, so there's nothing to adjust. That's something that should go in the documentation.

I'm planning to make an experimental branch with some of the optimizations found in the Kiwi library, and a lot of stuff removed, such as the begin/end edit calls, autosolve, perhaps even stay constraints, and I was doubting about change_strength as well.

Could you tell me a bit more about how you are using the library? I'm trying to decide whether to scratch this function, keep it, or extend it to support switching between required and soft constraints as well.

@ghost
Copy link
Author

ghost commented Dec 18, 2014

@Nocte- thanks for the reply. I give a bit more detail about how I am using the library in a comment in this thread: https://github.com/Nocte-/rhea/pull/37#issuecomment-67457993

Please ask if you need more details. Hopefully in a few weeks I might be able to show something. In principle, it seems to me a good idea to keep the function and to extend it for requires, since this system is very dynamic and it is possible to change properties at any time. But for now the work-around of removing/re-adding the constraint seems to work as long as auto-solve is off.

On your new branch: I really like the idea of removing begin/end_edit calls. It could be replaced with something where one puts a set of variables in editing mode with a scope object. Unlike the edit scope now, the scope itself tracks of the variables instead of the internal stack, thus allowing interleaved edits.

I also like removing auto_solve since I think it is nicer to work in general with it turned off. Although I had sometimes weird errors with auto_solve off... I can't reconstruct the case exactly, but I think it was related with resolve and solve being called in the right order. Probably unifying the two calls would be a nice simplification too!

I am curious about what you have in mind for stays :)

Please tell me if you need any help with the work or want me to try your branch with my system and test-suite.

Cheers!

@hfossli
Copy link
Collaborator

hfossli commented Dec 18, 2014

I love autosolve=off :D But I guess we have different needs. I can probably circumvent by having my own dispatcher...

@hfossli
Copy link
Collaborator

hfossli commented Dec 18, 2014

And I like the begin / end edit stuff. I don't just change one and one variable. I'm adding maybe 30 constraints at once. It makes then sense for the solver to solve when it gets the full picture (instead of after each single constraint).

@hfossli
Copy link
Collaborator

hfossli commented Dec 18, 2014

But I'm just a user of the algorithm / cassowary / rhea so I don't know if any of what I'm saying makes sense :D

@cacaodev
Copy link

Why don't you just disallow, or consider it is an error, to change from required to non-required and vice-versa. That's what Apple is doing with the NSLayoutConstraint priority property.

@Nocte-
Copy link
Owner

Nocte- commented Dec 18, 2014

@jbo-ableton: I had a short chat with the author of Kiwi (another Cassowary implementation). He removed stays completely because GUI systems are usually not underconstrained and don't really need them. It's an interesting thought and it seems to work fine for Enaml.

@hfossli: I'll check if batching is still worth it after I'm done with the redesign. If not, it's another bit of complexity I'd like to get rid of. (Probably it will only exist like in the current suggest(); it takes a list of suggestions and handles the edits for you.)

@cacaodev: I thought so, that's how the original implementation behaves as well. I was just wondering if jbo came across a use case where this would be useful. I think I'll disallow it for now and give it some more thought later.

@hfossli
Copy link
Collaborator

hfossli commented Dec 18, 2014

If it is excessive and leads to high complexity - let's get rid of it

@ghost
Copy link
Author

ghost commented Dec 18, 2014

@Nocte- I am fine with disallowing that, it's fine if it throws a exception.

About stays, if that is the only reason, I would not remove them. They are useful on dynamic systems, even for traditional GUY systems. For example, I am implementing a splitter element based on Rhea where the user can drag the handles to resize the elements. Stays are useful there. I might be able to show the code of a WIP version soon, I will keep you updated 😄

@hfossli
Copy link
Collaborator

hfossli commented Dec 18, 2014

Though it is more cumbersome - isn't it possible to acheive the same effect by removing a constraint and adding a new one?

@cacaodev
Copy link

From my experience (working on a js port of cocoa autolayout), there will always be a possibility that in a UI environment the root element will have ambiguous variables and something like a stay will be needed.

I've read a post in the cassowary.js mailing list where an Apple Engineer says that cocoa Autolayout doesn't have stays. So i've been digging in AppKit internals and it appears that the NSWindow (the root element) has in fact a constraint setup for the with and height. It's a regular NSLayoutConstraint in the form width == constant but this constant is updated at each resize of the window. This may be done internally in the solver (in this case it is very similar to a stay constraint) but it may also be done externally because cocoa Autolayout has the ability to setConstant: on a constraint already in the solver.

I think the key if we want to get rid of stay constraints but preserve the possibility to resolve ambiguities is to have the ability to set the constant on an expression already in the solver. It can be done already by removing/adding a constraint but this would be easier and i hope more efficient.

N.B: In cocoa autolayout, I believe this constraint has another purpose: the priority is 500 (NSLayoutPriorityWindowSizeStayPut) and it is the strength needed for a constraint to resize the window.

@Nocte-
Copy link
Owner

Nocte- commented Dec 21, 2014

@cacaodev Being able to easily change the constants of a constraint after adding it to the solver would be very useful indeed. I'll try to find out if there's an efficient way to do this.

@hfossli
Copy link
Collaborator

hfossli commented Dec 21, 2014

That would be really great!

ghost referenced this issue in Ableton/aqt-cassowary Jan 21, 2015
This uses the old and faulty re-add constraint technique because of
this issue:

    https://github.com/Nocte-/rhea/issues/33
ghost referenced this issue in Ableton/aqt-cassowary Jan 21, 2015
This uses the old and faulty re-add constraint technique because of
this issue:

    https://github.com/Nocte-/rhea/issues/33
ghost referenced this issue in Ableton/aqt-cassowary Jan 21, 2015
This uses the old and faulty re-add constraint technique because of
this issue:

    https://github.com/Nocte-/rhea/issues/33
ghost referenced this issue in Ableton/aqt-cassowary Jan 22, 2015
This uses the old and faulty re-add constraint technique because of
this issue:

    https://github.com/Nocte-/rhea/issues/33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants