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

[Android] [gui] Add initial support of Android in GGUI #3845

Merged
merged 1 commit into from
Dec 27, 2021

Conversation

ghuau-innopeak
Copy link
Collaborator

Related issue = #3679

@netlify
Copy link

netlify bot commented Dec 21, 2021

✔️ Deploy Preview for jovial-fermat-aa59dc canceled.

🔨 Explore the source changes: 86c8047

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/61c201d25fbab300088e18b0

@ghuau-innopeak
Copy link
Collaborator Author

This is an initial draft to allow using GGUI on Android, I'm not an entire fan of the changes in GGUI to run with Android but this is the simple transition I could do without impacting too much the code right now, the main issue is that GLFW code is a bit everywhere :).
Also, I'm still not 100% sure of the best solution for handling the DeviceAllocation, right now, I just added a new field in the struct as we cannot use the snode as it will require a program.

Here is a quick example on how to use it for Android as well: https://github.com/ghuau-innopeak/taichi-aot-tests/blob/main/android/app/src/main/cpp/main.cpp (it's test code... :))

@ghuau-innopeak ghuau-innopeak changed the title [Android][GUI] Add initial support of Android in GGUI [Android] [GUI] Add initial support of Android in GGUI Dec 21, 2021
@ghuau-innopeak ghuau-innopeak changed the title [Android] [GUI] Add initial support of Android in GGUI [Android] [gui] Add initial support of Android in GGUI Dec 21, 2021
@AmesingFlank
Copy link
Collaborator

This is amazing!
Regarding the device allocation, I'm happy with the current solution, but it's best if we add a comment to clarify when where the DeviceAllocation be used and when will the snode be used. (also, nit, I'd rename dalloc to dev_alloc)

@@ -144,7 +154,7 @@ void Renderable::create_graphics_pipeline() {
void Renderable::create_vertex_buffer() {
size_t buffer_size = sizeof(Vertex) * config_.max_vertices_count;

Device::AllocParams vb_params{buffer_size, false, false,
Device::AllocParams vb_params{buffer_size, true, true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I ask why this is needed for Android?
If this is really necessary, then we should guard it with #if ANDROID so that it doesn't hurt the performance of GGUI on other platforms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not needed, this is something I forget to revert during my testing... good catch! :D, it should be fixed.

Comment on lines 9 to 18
#ifdef ANDROID
#include <android/native_window.h>
typedef ANativeWindow TaichiWindow;
#else
typedef GLFWwindow TaichiWindow;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

tiny nit, let's use using A = B instead of typedef B A, and maybe put TaichiWindow inside taichi::ui?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! My habits of C developer :), I left the #include outside as well to avoid having all the functions from this header inside the Taichi namespace, I think I could have used namespace { #include <android/native_window.h> } instead to avoid the extra ifdef ANDROID but not sure what is preferred here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine.

@ghuau-innopeak ghuau-innopeak force-pushed the ggui_android branch 2 times, most recently from cd66776 to c45a6fa Compare December 21, 2021 16:21
@ghuau-innopeak
Copy link
Collaborator Author

@AmesingFlank Perfect :), I added a comment in the struct to clarify the usage of both fields.

Copy link
Collaborator

@AmesingFlank AmesingFlank left a comment

Choose a reason for hiding this comment

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

LGTM! Looking forward to seeing a demo on Android :D

Copy link
Collaborator

@bobcao3 bobcao3 left a comment

Choose a reason for hiding this comment

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

LGTM

@k-ye k-ye merged commit d56b424 into taichi-dev:master Dec 27, 2021
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