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

V4 support openharmony #20793

Open
wants to merge 5 commits into
base: v4-oh
Choose a base branch
from
Open

V4 support openharmony #20793

wants to merge 5 commits into from

Conversation

1-newbie
Copy link

V4 support openharmony

${ZLIB_LIBRARY}
${ICONV_LIBRARY}
"/usr/lib/libz.dylib"
"/usr/lib/libiconv.dylib"

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?

Copy link
Author

Choose a reason for hiding this comment

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

reverted

@@ -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)

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?

Copy link
Author

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)

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?

Copy link
Author

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

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?

Copy link
Author

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

@@ -10,8 +10,10 @@ require "cocos.init"

local director = cc.Director:getInstance()
local glView = director:getOpenGLView()
local widthx = 1024
local heighty = 2112

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)?

Copy link
Author

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)

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.

Copy link
Author

Choose a reason for hiding this comment

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

OHOS mean OpenHarmony OS

Copy link
Author

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);

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.

Copy link
Author

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);

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.

Copy link
Author

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;

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.

Copy link
Author

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);

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.

Copy link
Author

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,

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?

1-newbie and others added 2 commits May 27, 2024 17:17
fine

Co-authored-by: qiuguohua <qiuguohua111@126.com>
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.

None yet

3 participants