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

Update GLExtensions.cpp #26

Closed
wants to merge 2 commits into from
Closed

Update GLExtensions.cpp #26

wants to merge 2 commits into from

Conversation

Duron27
Copy link

@Duron27 Duron27 commented Jun 6, 2024

add GLES3

@AnyOldName3
Copy link
Member

This PR is pretty huge and definitely breaks things. E.g. it totally swaps out loads of OpenThreads stuff for STL threading primitives, but those aren't even available in C++03, which is what OSG uses. That particular change might be better dealt with by adding STL stuff as yet another backend for OpenThreads, or just figuring out how to make one of the several existing backends work (and at least one of them will do).

@elsid
Copy link

elsid commented Jun 30, 2024

For openmw we use C++11 to get move constructor for osg::ref_ptr but if there is no need to make a change, don't do it. And every change should be described in a separate commit message. What the change does and why it's required (what kind of problem it solves). If you apply a patch, reference it in the commit message with URL that leads to the patch of specific version. Commits with messages like "patches added" and "Update GLExtensions.cpp" don't explain what they do an why and very confusing for anyone looking at them without knowing the context.

Also this is blocked on the ability for CI to run on pull requests: #27 .

Copy link

@elsid elsid left a comment

Choose a reason for hiding this comment

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

So far linking openmw to this version of osg leads to a difference in the rendered picture:

This MR:

screenshot385

Upstream version:

screenshot386

@@ -735,7 +735,7 @@ OPTION(BUILD_OSG_PLUGINS "Build OSG Plugins - Disable for compile testing exampl
mark_as_advanced(BUILD_OSG_PLUGINS)
################################################################################
# 3rd Party Dependency Stuff
OPTION(OSG_FIND_3RD_PARTY_DEPS "Enable to search for Android or Windows dependencies in ./3rdparty" ON)
OPTION(OSG_FIND_3RD_PARTY_DEPS "Enable to search for Android or Windows dependencies in ./3rdparty" OFF)
Copy link

Choose a reason for hiding this comment

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

This likely will break all builds not setting this option explicitly. Set it in your cmake command not here.

Copy link
Author

Choose a reason for hiding this comment

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

I did not mean to add that commit from a few days ago. Can it be rolled back?

@@ -19,7 +19,7 @@ SET( COLLADA_ENV_VAR_AVAILABLE $ENV{COLLADA_DIR} )
IF ( COLLADA_ENV_VAR_AVAILABLE )
SET(COLLADA_DOM_ROOT "$ENV{COLLADA_DIR}/dom" CACHE PATH "Location of Collada DOM directory" FORCE)
ELSE ()
SET(COLLADA_DOM_ROOT "${ACTUAL_3DPARTY_DIR}/include/1.4/dom" CACHE PATH "Location of Collada DOM directory" FORCE)
SET(COLLADA_DOM_ROOT "${ACTUAL_3DPARTY_DIR}/include/collada-dom2.5/1.4/dom" CACHE PATH "Location of Collada DOM directory" FORCE)
Copy link

Choose a reason for hiding this comment

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

Collada 1.4 is unlikely located in the 2.5 unless something weird is done to setup packages. Please don't make this change.

@@ -74,6 +74,7 @@ FIND_PATH(COLLADA_INCLUDE_DIR dae.h
/opt/include
/usr/freeware/include
${ACTUAL_3DPARTY_DIR}/include
${ACTUAL_3DPARTY_DIR}/include/collada-dom2.5/
Copy link

Choose a reason for hiding this comment

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

There is a path suffix collada-dom2.5 just below which should make it possible to find collada 2.5. No need to have this change.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we're married to Collada DOM 2.4. 2.5 is really an alpha of the de-facto abandoned 3.x series, so has breaking changes.

@@ -163,7 +163,7 @@ MACRO(ANDROID_3RD_PARTY)
)
#set(ENV{AND_OSG_LIB_NAMES} "$ENV{AND_OSG_LIB_NAMES} libft2")
#set(ENV{AND_OSG_LIB_PATHS} "$ENV{AND_OSG_LIB_PATHS}include ${FREETYPE_DIR}/Android.mk \n")
set(FREETYPE_INCLUDE_DIRS ${FREETYPE_DIR}/include ${FREETYPE_DIR}/include/freetype/config)
set(FREETYPE_INCLUDE_DIRS ${FREETYPE_DIR}/include ${FREETYPE_DIR}/include/freetype2)
Copy link

Choose a reason for hiding this comment

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

This likely will break builds with freetype. What kind of problem this solves?

Comment on lines +304 to +306
# XXX: remove the "lib" prefix e.g. "libosgdb_bmp.a" => "osgdb_bmp.a"
# for some reason this is only a problem on android
SET_TARGET_PROPERTIES(${TARGET_TARGETNAME} PROPERTIES PREFIX "")
Copy link

Choose a reason for hiding this comment

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

What is the problem?

Comment on lines +164 to +167
#fix me
#IF(COLLADA_FOUND AND OSG_CPP_EXCEPTIONS_AVAILABLE)
# ADD_PLUGIN_DIRECTORY(dae)
#ENDIF()
Copy link

Choose a reason for hiding this comment

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

What is this for?

@Duron27
Copy link
Author

Duron27 commented Jul 1, 2024

Woa sorry, I had forgotten about this MR and added that patch for testing. I did not mean to include those in the MR at all

@Duron27 Duron27 closed this Jul 6, 2024
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.

3 participants