Skip to content

Conversation

@pailhead
Copy link
Contributor

@pailhead pailhead commented Jun 12, 2018

Added a little epsilon to get rid of the fuzziness.

#14197

@looeee
Copy link
Collaborator

looeee commented Jun 12, 2018

@pailhead
Copy link
Contributor Author

I don't know what's the right thing to do here, maybe the value could be smaller, maybe the cap mask could be scaled somehow, i know you can flip bits with floats i just dont know how to do it :(

@WestLangley
Copy link
Collaborator

What problem is this solving and how is this change solving it?

@pailhead
Copy link
Contributor Author

Someone posted that screenshot in the linked issue, this fixes it by moving the clipping of the endcaps slightly towards the end caps. If the value is small enough this shouldn't show up, if it's larger there could be some artifacts. But it solves the fuzziness from the imprecise value.

@pailhead
Copy link
Contributor Author

Did it fix it for you @looeee , the link works for me?

@looeee
Copy link
Collaborator

looeee commented Jun 12, 2018

Yes, it nearly completely removed the fuzzy gaps in the lines for me.

@WestLangley here's the screenshot from #14197 that shows the problem:

image

@pailhead
Copy link
Contributor Author

Maybe this could benefit from a different approach, this is a bandaid, I think there needs to be a distinct value in the body.

@Usnul
Copy link
Contributor

Usnul commented Jun 12, 2018

I still see some breakage, but it's rather minimal.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 12, 2018

@pailhead How did you determine the values 0.49999 and 0.50001?

@pailhead
Copy link
Contributor Author

I didn’t, I imagined the numbers are just stretched over those middle polygons, and tried to shift the clipping. I need to study this a bit more.

@pailhead
Copy link
Contributor Author

I thought i could get rid of the epsilon if make an uv split for the body, But it didn't quite work, i think it has a problem with checking for that 0.5 value without even the interpolation.

In this last diff i added another attribute value, 0 or 1 if it's end cap or not.

@pailhead
Copy link
Contributor Author

pailhead commented Jun 12, 2018

@Mugen87

I don't think using the values 0.49999 and 0.50001 is robust since such tolerance values significantly depend on the scale of the geometry. I assume they are chosen more or less arbitrary right now. They maybe solve this particular example but not others.

It doesn't actually depend on the scale of the geometry. This is all in map space, you can imagine the segment as new THREE.PlaneBufferGeometry(3,1,3,1).

I think overall that these equality checks for floats don't work with shaders but i don't quite understand why. Whenever you want to check for a float, it's good to make it fuzzy. In this last example that middle segment is guaranteed to have the what's supposed to be 0.5 value, but it still didn't work.

Check out this mindblowing thing:

Will not cause artifacts, even though _EPSILON is just 0

const float _EPSILON = 0.000000;
const float SEG_START = 0.5 - EPSILON;
const float SEG_END = 0.5 + EPSILON;

Will cause artifacts.

const float SEG_START = 0.5;
const float SEG_END = 0.5;

You'd need to check out 30bbf60

@pailhead
Copy link
Contributor Author

Slightly different results, from #14279, maybe we could look into not clipping all the caps for dashed.

@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2018

Merged #14275 instead.

@mrdoob mrdoob closed this Jun 13, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2018

Thanks!

@pailhead
Copy link
Contributor Author

pailhead commented Jun 13, 2018

btw why did we need another PR for this? Wouldn't it be easier to review one? @mrdoob @WestLangley.

As is there are two slightly different approaches, and anyone interested in this has no breadcrumbs as to why is one preferred over the other. Doesn't matter which one, but what's in them, what are the implications of pushing more data into attributes, more computation here vs there etc.

What's the role of this PR, since #14197 outlined really well what the problem is, but it wasnt enough to make #14275 happen. It appeared after posting this PR.

What problem is this solving and how is this change solving it?

@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2018

btw why did we need another PR for this? Wouldn't it be easier to review one? @mrdoob @WestLangley.

Could we just move on instead of discussing this?

There are a lot of pending PRs that are being delayed because I'm trying to catch up with the extensive discussions that have been happening over the previous days 😟

@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2018

If you think your PR was better than the one that was merged I'd be happy to discuss that. Otherwise, I would prefer to keep moving.

@pailhead
Copy link
Contributor Author

pailhead commented Jun 13, 2018

If you think your PR was better

@mrdoob
I think it's not obvious, it would be beneficial to discuss it. I'm not sure if either is better. The other one has three times as many checks going on (9 vs 3). I don't actually know how this impacts things.

I think it might be better just on account of this, but it also yielded slightly different visual results. I rely a lot on thick lines, in my day to day life, i'd like to follow the development of this.

It's overall hard to dial down and understand this things.

@WestLangley
Copy link
Collaborator

@pailhead Thank you for pointing out that the following change in your code removed the artifacts for you:

if ( vUv.y < 0.49999 || vUv.y > 0.50001 ) discard;

It was then clear to me that an attribute that was assumed to be a constant 0.5 across the face of the primitive was not guaranteed to be so in practice -- at least not on certain hardware.

My solution in #14279 was to change the model so the assumption of the constant attribute was no longer required. This was an easy and trivial change. I filed a PR because that was my preferred solution.


On a related matter, this "fat line" feature was difficult to get right, and the implementation is not easy to understand. Consequently, I wrote the code with clarity in mind. The only reason to further modify the code for performance reasons is if there is a demonstrated performance issue.

@pailhead
Copy link
Contributor Author

Fair enough, this does make sense. Doing bool isSomething; might make sense too though, from a readability standpoint. Thanks for clarifying.

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

Successfully merging this pull request may close these issues.

6 participants