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

default chordTol=0.1% max segments=20 #627

Closed
wants to merge 4 commits into from

Conversation

phkahler
Copy link
Member

@phkahler phkahler commented Jun 4, 2020

No description provided.

@phkahler
Copy link
Member Author

phkahler commented Jun 7, 2020

I actually think 0.1 percent is better and that's what I try to use. I thought 0.2 might help for people with older machines but it still looks a little jagged to me.

This and the other PR for circles are breaking tests due to visual differences, so not stuff to keep changing.

@ruevs
Copy link
Member

ruevs commented Jun 8, 2020

What was the problem with the current 0.5, 10 ?

In most cases I also use it at 0.1, 40 but it quickly becomes unpleasantly slow with more complex models even on a fast 8 core machine with a fairly modern GPU.

To me it is an unclear trade-off whether we should be "good looking but slow" or "coarse and fast" by default. In both cases some new users will complain until they discover the configuration options.

@phkahler
Copy link
Member Author

phkahler commented Jun 8, 2020

Warning, massive opinion piece here:

For reference I just got out my 2015 MacBook Air with a 1.6GHz Intel thing and graphics. It runs version 2.3 and was used to design the ITX case on my github. I had left the settings at 0.01 and way too many segments. It runs OK. GPU is great for zooming and rotating the model. Changing the chord tolerance to 0.1 is OK but it runs into the octagon holes problem and performance doesn't improve much.

I think the setting are not intuitive. One might think that increasing the max segments is going to improve the look, but if your CT is set to 0.5 it's not going to do much. Having chord tolerance set really low like 0.001 to 0.01 and then using the max segments to change the coarseness would be more intuitive but probably a bad idea. The primary adjustment should be CT and it will probably make most things look smooth with no more than 20 segments (which is 80 for a circle - probably 128 the way it's actually implemented). I certainly don't think 0.5 and 10 were good choices as they are both adjusted for performance and both need to be changed to improve visual quality.

So I think CT should be the primary adjustment, but max segments should be set between 20 and 40 or people will absolutely kill performance for no benefit while lowering CT. I think 0.1 is substantially better than 0.5 for appearance without killing performance. And 20 segments might be low but it does allow significant visual improvement by lowering CT only and might prevent people from killing performance going crazy with low CT values - which might be needed for large sketches with small detail.

Along with the changes to prevent octagons I think changing the defaults will make people much less likely to ask about these things. Recent performance improvements may help a little with the smoother appearance settings too.

It's the new people I worry about and I don't think the defaults are good for anyone.

FYI my own development setup is a Mellori ITX with an AMD 2400G 4-core, 8-thread, with integrated graphics (the very one shown on github). The GPU is never stressed by SolveSpace. I use a Samsung 55" 4K curved TV as a monitor and I can (but don't) run SolveSpace full screen with no problems from the GPU. The slowdowns happen when CT is reduced too far and regeneration takes a long time. We could use more multi-threaded code ;-)

Just my opinions. After testing on my old Mac I think I'm going to push a change here to make default 0.1 percent in case this PR does get merged.

@phkahler phkahler changed the title default chordTol=0.2% max segments=20 default chordTol=0.1% max segments=20 Jun 8, 2020
@ruevs
Copy link
Member

ruevs commented Jun 9, 2020

@phkahler Your argumentation makes perfect sense. Both values are very useful and I agree with you on how they should be used.
I actually meant to write that I use a chord tolerance of 0.01 and 40 segments. And I do not have a problem with moving and rotating the model at all, but as you said "regeneration takes a long time" and this is what makes big models "unpleasantly slow" for me since (at least the way I use it) re-generation happens very often.

Ultimately every user should learn that these two options exist and use them. So our discussion is only about the "new user experience". In fact I am fine with any defaults, but it is worth changing them rarely since (as you mentioned) the test cases have to be updated :-)

@phkahler
Copy link
Member Author

phkahler commented Jun 9, 2020

Ultimately every user should learn that these two options exist and use them. So our discussion is only about the "new user experience".

@ruevs Agreed, I just don't want them to have to learn about this right away.

If you use 0.01 and 20 (rather than 40) does it help performance? does it hurt the visual? From a new user point of view, which would be better given a starting CT of 0.1 that they then lower?

I'd say maybe we could just increase max segments and still pass the tests, but they will need to be updated for the octagon holes issue anyway. May as well pick new values all at once?

I just made the bracket from the demo video on the main SolveSpace web site using default settings and ran into the octagon holes issue. Setting CT = 0.2 helped and 0.1 even more. It also helps with the arc on the gusset. There is no "right" set of default values but I do think it currently has a "wrong" set of values and should be improved. Noobs should not follow the tutorial and run into that stuff.

@Evil-Spirit
Copy link
Collaborator

After OpenGL renderer improvements SolveSpace is totally CPU bounded. Don't even except GPU bound since you can't create such complex models because of slow geometry processing. If you wanted to get some speed in creation of complex models, you should use/integrate my branches.

@phkahler
Copy link
Member Author

@Evil-Spirit Agreed that your OpenGL work has made the render times so low as to be irrelevant :-) One of the first things I thought SolveSpace needed was named parameters and I have watched your video on that work. It looks really impressive and I still plan to take a closer look at the code, but it does seem a little incomplete. Some of your other changes are still sitting around as "patch-available" and I wonder why (aside from whitequark being busy). This PR is really for new user happiness more than anything.

@whitequark I think these two are good to go but I don't know how to update the test cases.

@whitequark
Copy link
Contributor

aside from whitequark being busy

That's the primary reason.

I think these two are good to go but I don't know how to update the test cases.

When you run the tests locally, for failing tests, you get an actual result next to the reference result. Rename the former to the latter, done.

@phkahler
Copy link
Member Author

When you run the tests locally, for failing tests, you get an actual result next to the reference result. Rename the former to the latter, done.

Thanks @whitequark I knew I could count on you to teach me to fish. Now to find those 23 changed images.... ;-)

@whitequark
Copy link
Contributor

Now to find those 23 changed images.... ;-)

I have this file called commit.sh:

#!/bin/sh -ex

make -C build solvespace-testsuite
./build/test/solvespace-testsuite $* || true
for e in slvs png; do
  for i in `find . -name *.out.$e`; do
    mv $i `dirname $i`/`basename $i .out.$e`.$e;
  done;
done

but it's kind of too hacky to have in the repo.

@phkahler
Copy link
Member Author

@whitequark I'll leave this for you to have time to review the previous one.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Jun 13, 2020

One of the first things I thought SolveSpace needed was named parameters

Just need to do some UI.

@whitequark whitequark force-pushed the master branch 2 times, most recently from 4c00bde to c876104 Compare June 14, 2020 05:13
whitequark pushed a commit that referenced this pull request Jun 14, 2020
@whitequark
Copy link
Contributor

Merged manually as 3d51b39.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants