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

[GUI] Fix issue with triangles not showing up when they are drawn clockwisely #1321

Merged

Conversation

Rullec
Copy link
Contributor

@Rullec Rullec commented Jun 24, 2020

Hi all,

One of our classmate in GAMES201 from Wechat found that ONLY when [va, vb, vc] is in counterclockwise order can ti.gui.triangle(va, vb, vc) manage to display it. If the offered vertices are in clockwise, our ti.GUI simply ignore it.

For example, if I say

    triangle=[(0.3,0.5),(0.4,0.7),(0.5,0.55)]
    gui.triangle(*triangle,color=0xff0000)

This triangle won't be drawn.

If it is a designed feature, maybe we can try to doc it?
If not, here is one possible solution.

HDYT? Maybe we can have a better solution.

@Rullec
Copy link
Contributor Author

Rullec commented Jun 24, 2020

@Eydcao proposed that, it can also be implented by barycentric coordinate. Also a good solution!

@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #1321 into master will decrease coverage by 0.40%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1321      +/-   ##
==========================================
- Coverage   85.76%   85.35%   -0.41%     
==========================================
  Files          18       19       +1     
  Lines        3294     3374      +80     
  Branches      624      630       +6     
==========================================
+ Hits         2825     2880      +55     
- Misses        343      362      +19     
- Partials      126      132       +6     
Impacted Files Coverage Δ
python/taichi/lang/impl.py 90.06% <0.00%> (-0.14%) ⬇️
python/taichi/lang/shell.py 40.67% <0.00%> (ø)
python/taichi/lang/kernel.py 80.82% <0.00%> (+0.04%) ⬆️
python/taichi/lang/__init__.py 75.35% <0.00%> (+0.35%) ⬆️
python/taichi/lang/ast_checker.py 72.22% <0.00%> (+0.39%) ⬆️
python/taichi/lang/matrix.py 90.67% <0.00%> (+0.50%) ⬆️
python/taichi/lang/expr.py 89.75% <0.00%> (+1.69%) ⬆️
python/taichi/lang/snode.py 86.02% <0.00%> (+8.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9c9616...09d10b0. Read the comment docs.

@archibate archibate marked this pull request as ready for review June 24, 2020 15:07
taichi/gui/gui.cpp Outdated Show resolved Hide resolved
@Rullec Rullec requested a review from archibate June 24, 2020 16:17
@Eydcao
Copy link
Contributor

Eydcao commented Jun 24, 2020

@Eydcao proposed that, it can also be implented by barycentric coordinate. Also a good solution!

Thanks~ I was in a meeting just now. I have reviewed your code and find the solution good enough. It turns out it does not matter whether the cross product is positive or negative (or say the particle is at the left side or the right side of the edge), as long as the it is at the same side for all edges, it can only be inside.

So this is good enough already. Good fix!

@Rullec
Copy link
Contributor Author

Rullec commented Jun 24, 2020 via email

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Thank you! Originally I designed the API to ignore clock-wise triangles, but since it is confusing the users, let's just take your proposal.

@yuanming-hu yuanming-hu merged commit 3ebba1a into taichi-dev:master Jun 25, 2020
Rullec added a commit to Rullec/taichi that referenced this pull request Jun 26, 2020
@yuanming-hu yuanming-hu mentioned this pull request Jun 28, 2020
@Rullec Rullec deleted the draw_triangle_failed_in_couterwise branch July 26, 2020 16:08
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.

4 participants