Skip to content

Conversation

joliver82
Copy link
Contributor

@joliver82 joliver82 commented Mar 10, 2021

This PR fixes issue #1239 (both sound not working and ogg files not loaded).

Also this PR updates openal-soft to latest (1.21.1).

EDIT: I saw it's failing to compile. Maybe this is caused by an old release of the ndk, I'm using latest (22)

joliver82 and others added 16 commits May 17, 2018 23:07
… in the atlases instead of setting a predefined byte buffer

on disposal that made all atlases in the backend use the same buffer and generated rendering issues.
…d more images to the screens so it really uses more atlases
…loading fail and no sound on android

Fixed bad path check in openalsoft.gradle
Updated openal-soft to 1.17.2 and added related required defines in config.h
Included new required header files
Updated target platform to android-19 (previous just don't link)
@Ali-RS
Copy link
Member

Ali-RS commented Mar 10, 2021

Thanks, @joliver82

By the way, what is bsinc_inc.h for?

@Ali-RS Ali-RS added this to the v3.4.0 milestone Mar 10, 2021
@joliver82
Copy link
Contributor Author

bsinc_inc.h is an autogenerated header file for openal-soft 1.19.1 (see commit joliver82@fc99412) for 1.21.1 is useless and I've just removed it

@MeFisto94
Copy link
Member

MeFisto94 commented Mar 10, 2021

Well, you bumped the Target Version from 9 to 19, so that certainly requires a more recent NDK.
It may be worth asking @riccardobl to update

image: riccardoblb/buildenv-jme3:android
and it's integrated NDK version.

This is something that may even fix the compiler issues.
The "optimal" solution here would be to look at the relevant bytecode and find out why optimization fails, maybe with the help of some clang/gcc guys. Maybe we can make it more clear from the source code to prevent that issue, if it's not fixed in a more recent version already.

I mean the compiler shouldn't like change values into -1 randomly.
That may however be done after this workaround, but having Optimizations, especially on mobile devices, is a good idea.

Regarding the build failures, the first one is /opt/android/ndk-bundle/build/ndk-build: line 148: file: command not found
And then: https://stackoverflow.com/questions/64281680/error-no-template-named-enable-if-t-in-namespace-std-did-you-mean-enable
-> As the Warnings also indicate, we need to enable the C++14 featureset, which may or may not already work with that NDK version, given the correct flags

@joliver82
Copy link
Contributor Author

joliver82 commented Mar 10, 2021

Android api level 19 stands for kitkat 4.4 which was released on 2013 so I assume riccardobl's image is new enough to support it.

I agree that it would be better to properly fix this keeping optimizations enabled but I'm not that good at C/C++ and I didn't want to change code from external libraries (the issues are located at tremor and openal-soft)

About C++14, if setting any -str=c++* flag ndk-build fails because there're plain C files (com_jme3_*.c) in the same library, that's why I didn't. Also it compiled properly in ndk 22

EDIT:

the line 148 at ndk-build is caused because "file" is not installed in the docker image:
line 148: file -L "$SHELL" | grep -q "x86[_-]64"

The ndk in the image is 20.0.5594570, in fact updating the ndk succeeded to compile it. I've been reading about this and although c++14 is supported and is clang's default, it was not working this way in ndk till r21:

https://github.com/android/ndk/wiki/Changelog-r21
--> Fixed ndk-build to use Clang's default C++ standard version (currently C++14) when using libc++.

it would be great if @riccardobl could just rebuild the riccardoblb/buildenv-jme3:android docker image so we would use latest ndk to build jme3 android natives (and also add file package to avoid the ndk line 148 error)

@Ali-RS
Copy link
Member

Ali-RS commented Mar 13, 2021

it would be great if @riccardobl could just rebuild the riccardoblb/buildenv-jme3:android docker image so we would use latest ndk to build jme3 android natives (and also add file package to avoid the ndk line 148 error)

I am not familiar with JME's build env but can this be configured from https://github.com/jMonkeyEngine/buildenv-jme3?

And about this:

image: riccardoblb/buildenv-jme3:android

Should we change it to use jmonkeyengine/buildenv-jme3:android? (https://hub.docker.com/r/jmonkeyengine/buildenv-jme3)

@riccardobl
Copy link
Member

it would be great if @riccardobl could just rebuild the riccardoblb/buildenv-jme3:android docker image so we would use latest ndk to build jme3 android natives (and also add file package to avoid the ndk line 148 error)

I am not familiar with JME's build env but can this be configured from https://github.com/jMonkeyEngine/buildenv-jme3?

And about this:

image: riccardoblb/buildenv-jme3:android

Should we change it to use jmonkeyengine/buildenv-jme3:android? (https://hub.docker.com/r/jmonkeyengine/buildenv-jme3)

Yes. It should be changed to jmonkeyengine/buildenv-jme3:android (from https://hub.docker.com/u/jmonkeyengine) .
As you have noticed I mirrored my repo to the jme org, but i've never actually changed the references in the build script.

@joliver82
Copy link
Contributor Author

I just changed it. The image was built 12 days ago, so it should have latest ndk and compile properly

@MeFisto94
Copy link
Member

@Ali-RS
Copy link
Member

Ali-RS commented Mar 13, 2021

Note that /opt/android/ndk-bundle/build/ndk-build: line 151: file: command not found is still happening. In case this is still an issue!

@joliver82
Copy link
Contributor Author

Not a real issue, but I'll add file package to the docker image later

@Ali-RS
Copy link
Member

Ali-RS commented Mar 13, 2021

Thanks

@stephengold
Copy link
Member

Is this PR ready to be integrated?

@joliver82
Copy link
Contributor Author

joliver82 commented Mar 13, 2021

Note that /opt/android/ndk-bundle/build/ndk-build: line 151: file: command not found is still happening. In case this is still an issue!

Created a PR for buildenv to fix this (jMonkeyEngine/buildenv-jme3#1)

Is this PR ready to be integrated?

From my point of view, it is

@stephengold stephengold merged commit 8c4d523 into jMonkeyEngine:master Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants