-
Notifications
You must be signed in to change notification settings - Fork 7k
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
V4 support openharmony #20793
base: v4-oh
Are you sure you want to change the base?
V4 support openharmony #20793
Conversation
${ZLIB_LIBRARY} | ||
${ICONV_LIBRARY} | ||
"/usr/lib/libz.dylib" | ||
"/usr/lib/libiconv.dylib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to modify the ios platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
cmake/Modules/CocosBuildSet.cmake
Outdated
@@ -29,7 +29,7 @@ message(STATUS "PYTHON_PATH:" ${PYTHON_COMMAND}) | |||
message(STATUS "COCOS_COMMAND_PATH:" ${COCOS_COMMAND}) | |||
message(STATUS "HOST_SYSTEM:" ${CMAKE_HOST_SYSTEM_NAME}) | |||
# the default behavior of build module | |||
option(BUILD_LUA_LIBS "Build lua libraries" OFF) | |||
option(BUILD_LUA_LIBS "Build lua libraries" ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it need to be on by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
|
||
#include "base/CCController.h" | ||
|
||
#if (CC_TARGET_PLATFORM == CC_PLATFORM_OHOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a file only owned by the openharmony platform and does not need to be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just keep code style, android and ios also had this
@@ -36,7 +36,13 @@ | |||
#include "base/CCDirector.h" | |||
#include "base/CCEventCustom.h" | |||
|
|||
#pragma comment(lib,"lua51.lib") | |||
#if (CC_TARGET_PLATFORM == CC_PLATFORM_OHOS) | |||
#if _MSC_VER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
‘_MSC_VER‘ is a windows macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but if not modify this, it will compile error, so to avoid influence other platform, we add marco
tests/lua-tests/src/controller.lua
Outdated
@@ -10,8 +10,10 @@ require "cocos.init" | |||
|
|||
local director = cc.Director:getInstance() | |||
local glView = director:getOpenGLView() | |||
local widthx = 1024 | |||
local heighty = 2112 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to change this width and height for other platforms (like windows, android)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
@@ -105,6 +105,78 @@ elseif(ANDROID) | |||
audio/android/tinysndfile.cpp | |||
) | |||
|
|||
elseif(OHOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does OHOS
mean harmonyOS or openharmony? It's really confused to me.
Look at this in Cocos Creator: https://github.com/cocos/cocos-engine/blob/v3.8.2/native/CMakeLists.txt#L114
In Cocos Creator, OHOS macro is for HarmonyOS while OPENHARMONY macro is for openharmony OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OHOS mean OpenHarmony OS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HarmonyOS should be HMOS
void MessageBox(const char * pszMsg, const char * pszTitle) { | ||
std::string msg(pszMsg); | ||
std::string title(pszTitle); | ||
JSFunction::getFunction("DiaLog.showDialog").invoke<void>(msg, title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should DiaLog.showDialog
be Dialog.show
? Dialog
is a word and doesn't need to make the L
uppercase.
And show
has already been in the scope of Dialog, the name of showDialog
is a little redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the DiaLog.showDialog is defined at NapiHelper.ts in template project, just a string used to point to the function. We have declared many functions with similar styles in NapiHelper.ts. So I think there is no need to change this.
*/ | ||
virtual ~FileUtilsOhos(); | ||
|
||
static void setassetmanager(NativeResourceManager* a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to keep the same naming rules.
Here should be setAssetManager
like the following getter getAssetManager
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary assetmanager, deleted
#include <EGL/egl.h> | ||
PFNGLGENVERTEXARRAYSOESPROC glGenVertexArraysOESEXT = 0; | ||
PFNGLBINDVERTEXARRAYOESPROC glBindVertexArrayOESEXT = 0; | ||
PFNGLDELETEVERTEXARRAYSOESPROC glDeleteVertexArraysOESEXT = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use nullptr
to initialize a pointer variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with android
|
||
auto fileUtils = FileUtils::getInstance(); | ||
std::vector<std::string> searchPaths; | ||
|
||
if (screenSize.height > 320) | ||
{ | ||
auto resourceSize = Size(960, 640); | ||
auto resourceSize = Size(1024, 2112); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the default value of design resolution size and resource size?
The design resolution and content scale factor should not be changed since they were confirmed when write the test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
blockSize, TextHAlignment::LEFT, verticalAlignment[vAlignIdx]); | ||
auto center = Label::createWithTTF("alignment center", fontFile, fontSize, | ||
blockSize, TextHAlignment::CENTER, verticalAlignment[vAlignIdx]); | ||
auto right = Label::createWithTTF("alignment right", fontFile, fontSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Label::createWithSystemFont
was not implemented for openharmony platform?
fine Co-authored-by: qiuguohua <qiuguohua111@126.com>
V4 support openharmony