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

Feature: option to build tcvm without Skia #287

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

erathke
Copy link
Contributor

@erathke erathke commented Jan 26, 2021

Description:

New feature to build TCVM without Skia.
It's possible to generate a very little final libtcvm (~3MB on Linux).
It's also now possible to test the VM on OpenBSD.

Motivation and Context:

Able to use TCVM on OpenBSD.

Benefited Devices:

  • OS: OpenBSD 6.8

Tested Devices:

  • OS: Linux Debian 10

Copy link
Contributor

@acmlira acmlira left a comment

Choose a reason for hiding this comment

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

Hello! Hey, first of all, thanks for your contribution! I was going to start reviewing your PR and I came across CI errors:

  1. Commit message error how to solve it?
  2. Android build error: I observed in your commit that you changed something in the build files that impacted the Android build. So can you try to solve this problem? If you can't I can help you 😅

You can create new commits or simply force push your PR branch we can do squash after approval.

@erathke
Copy link
Contributor Author

erathke commented Jan 27, 2021

Hi @acmlira ! How are you? I hope you are well.
I did not change the code at that specific place. I got the error and compare with the HEAD and I couldn't find any changes.
If the SKIA_H is defined then the function fadeScreen is available:

nm/ui/gfx_Graphics.c

#ifdef SKIA_H
// Darkens the screen
static void fadeScreen(Context currentContext, int32 amount) {
skia_fillRect(0, 0, 0, screen.screenW, screen.screenH, amount << 24);
currentContext->dirtyX1 = 0;
currentContext->dirtyY1 = 0;
currentContext->dirtyX2 = screen.screenW;
currentContext->dirtyY2 = screen.screenH;
}
#endif

Please, let me know if you need another action from myself.
Thanks in advance.

Evandro.

@teras
Copy link
Collaborator

teras commented Jan 27, 2021

Maybe the android build was affected because it always requires Skia?

@erathke
Copy link
Contributor Author

erathke commented Jan 27, 2021

Maybe the android build was affected because it always requires Skia?

Hi @teras!
By default Skia is enabled, unless defined in cmake (-DUSE_SKIA=OFF).
Android build uses gradle and not cmake...

@acmlira
Copy link
Contributor

acmlira commented Jan 27, 2021

We have a native part for Android. This build is called by gradle, underneath, gradle calls CMake to build this big part.

@erathke
Copy link
Contributor Author

erathke commented Jan 27, 2021

We have a native part for Android. This build is called by gradle, underneath, gradle calls CMake to build this big part.

Thanks for the clarification!
I found the issue. I'm working on it.

@acmlira
Copy link
Contributor

acmlira commented Jan 29, 2021

Okay, now we just need to adjust the commit message :)

@erathke
Copy link
Contributor Author

erathke commented Jan 29, 2021

Okay, now we just need to adjust the commit message :)

Hi @acmlira !
What we need for a good commit message? :)

@acmlira
Copy link
Contributor

acmlira commented Jan 29, 2021

@erathke
Copy link
Contributor Author

erathke commented Jan 29, 2021

Here "Commit message error how to solve it?" :)

What do you think?

feature: option to build tcvm without skia

This feature ables to build TCVM without Skia. By default, this option is ON.
Without Skia, is possible to generate a very little final libtcvm
(~3MB on Linux).
It's also now possible to test the VM on OpenBSD.
Usage - Disable Skia: cmake <source_dir> -DUSE_SKIA=OFF

@acmlira
Copy link
Contributor

acmlira commented Jan 29, 2021

That's it! Thank you very much for your contribution 😄

@flsobral
Copy link
Member

flsobral commented Feb 7, 2021

OMG I finally took the time to check your PR and understood what you did.
You managed to disable Skia in a way that our legacy drawing operations would work instead. That's really neat!
You're using the original graphical implementation, that nowadays is only used on Windows and Windows CE.
This implementation supports the usage of accelerated graphics (to a certain degree at least), but to use it you must make sure SDL is initialized with a GL context and find a way to share this context with this graphical implementation (while also defining _gl2_h) and it should work.
Take a look at gfx_Graphics_c.h in the android folder and mainview.m to see how the GL context is initialized on Android and iOS.

The only thing you'll be missing is true type font support, you'll be limited to the tcz fonts.

@erathke
Copy link
Contributor Author

erathke commented Feb 7, 2021

Thanks @flsobral for your feedback! I think that the acceleration can be implemented in another PR (actually I'm working on it). The target of this PR is to disable Skia and I think that is enough for now, if you agree, of course.
Please, let me know if I need to take another action. :)

@erathke
Copy link
Contributor Author

erathke commented Mar 14, 2021

Hello @acmlira! How are you?
Will you accept this PR? I ask you because I have others on my queue.
Thanks in advance.
Evandro

Copy link
Member

@flsobral flsobral left a comment

Choose a reason for hiding this comment

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

@erathke could you please rebase your branch on top of our master and squash your commits into a single one?
I'll test the builds for iOS and Windows to accept your PR as soon as you do that. ;)

This feature ables to build TCVM without Skia. By default, this option is ON.
Without Skia, is possible to generate a very little final libtcvm
(~3MB on Linux).
It's also now possible to test the VM on OpenBSD.
Usage - Disable Skia: cmake <source_dir> -DUSE_SKIA=OFF
@erathke
Copy link
Contributor Author

erathke commented Apr 8, 2021

Hello @flsobral ! Done :D

@flsobral flsobral self-assigned this Apr 23, 2021
@flsobral flsobral merged commit 278a1df into TotalCross:master Apr 23, 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.

5 participants