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

[OpenGL] [sparse] Basic support for dynamic SNode #1256

Merged
merged 12 commits into from
Jun 26, 2020

Conversation

archibate
Copy link
Collaborator

@archibate archibate commented Jun 16, 2020

Related issue = #711

[Click here for the format server]


Basic support!

  1. Use for i in range(dynamic_len): list[i] = i for listgen (low efficiency but works).
  2. No non-top dynamic snode like ti.root.dense.dynamic (non-trivial, will do iapr).

I also noticed that making msgbuf[] size to be explicit cause NV GLSL compiler to be slow. Should I open an extra buf for msg to make NV quicker? It may push mesa GL more close to it's buf number limit of 12.

@archibate archibate changed the title [skip ci] [OpenGL] [sparse] Try to implement dynamic SNode [OpenGL] [sparse] Try to implement dynamic SNode Jun 16, 2020
@archibate archibate added the error-prone This PR may introduce potential bug if not carefully reviewed & tested label Jun 16, 2020
@archibate archibate self-assigned this Jun 16, 2020
@archibate archibate requested review from k-ye and yuanming-hu and removed request for yuanming-hu June 16, 2020 07:10
@archibate archibate changed the title [OpenGL] [sparse] Try to implement dynamic SNode [OpenGL] [sparse] Basic support for dynamic SNode Jun 16, 2020
@archibate archibate marked this pull request as ready for review June 16, 2020 16:12
@archibate archibate added opengl OpenGL backend and removed skip ci labels Jun 16, 2020
@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #1256 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
+ Coverage   85.35%   85.48%   +0.12%     
==========================================
  Files          19       19              
  Lines        3374     3375       +1     
  Branches      630      630              
==========================================
+ Hits         2880     2885       +5     
+ Misses        362      358       -4     
  Partials      132      132              
Impacted Files Coverage Δ
python/taichi/lang/expr.py 89.82% <100.00%> (+0.06%) ⬆️
python/taichi/lang/util.py 64.07% <0.00%> (+2.39%) ⬆️

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 13462ff...5afb63b. Read the comment docs.

@archibate
Copy link
Collaborator Author

Hello? I thought I made a big step..

@archibate
Copy link
Collaborator Author

Hello? 7 days passed and nobody react me except bots.

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry, back then I was wondering whether it's good to support a somewhat limited dynamic. I'd also like to hear @yuanming-hu 's opinion on this. Can we discuss on this kind of decision before implementing it next time?

@@ -2,6 +2,10 @@


def ti_support_dynamic(test):
Copy link
Member

Choose a reason for hiding this comment

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

Add a test case for top-level dynamic?

taichi/backends/opengl/codegen_opengl.cpp Show resolved Hide resolved
taichi/backends/opengl/codegen_opengl.cpp Show resolved Hide resolved
@archibate
Copy link
Collaborator Author

LGTM! Sorry, back then I was wondering whether it's good to support a somewhat limited dynamic. I'd also like to hear @yuanming-hu 's opinion on this. Can we discuss on this kind of decision before implementing it next time?

The limited support here is good enough for ordinary purpose. Taichi THREE for example, only wants extend-able array for loading meshes.

@archibate
Copy link
Collaborator Author

@yuanming-hu Could you take a pass? It's becoming stale.

@archibate archibate added the c++ C++ engineering related label Jun 25, 2020
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.

Thanks for supporting dynamic here for OpenGL. I think it's very helpful in the short term. In the long term, we are refactoring the dynamic API, so I think we should stick to a basic implementation in OpenGL and switch to a more sophisticated implementation of the new system later.

Due to time constraints I'm sorry that I won't have time to take a close look at your implementation. Please feel free to merge.

@archibate archibate merged commit a3b8d28 into taichi-dev:master Jun 26, 2020
@yuanming-hu yuanming-hu mentioned this pull request Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ C++ engineering related error-prone This PR may introduce potential bug if not carefully reviewed & tested opengl OpenGL backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants