axmolengine / axmol

Axmol Engine – A Multi-platform Engine for Desktop, XBOX (UWP) and Mobile games. (A fork of Cocos2d-x-4.0)
https://axmol.dev
MIT License
922 stars 205 forks source link

Android getSafeAreaRect() Wrong values #2175

Closed AlexandreK38 closed 1 month ago

AlexandreK38 commented 1 month ago
  1. Call Director getSafeAreaRect() and note the given values.
  2. Call Director getSafeAreaRect() once again and check for values, these are not the same as previous call.

The cause comes from the static var below, the first time the JNI is called and values computed and stored in the static array, but the next call the JNI is not called and the values are 0.

static int* cornerRadii =
            JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");
static int* safeInsets =
            JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getSafeInsets");

It seems to an issue with static variable in java. The fix is just to remove the static keyword.

AlexandreK38 commented 1 month ago

@rh101 Hi! I saw you did the PR #1934, didn't you notice this behavior?

rh101 commented 1 month ago

@rh101 Hi! I saw you did the PR #1934, didn't you notice this behavior?

I recall making the change to support the rounded corners, and added the call: static int* cornerRadii = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

The static int* safeInsets isn't new; it's been that way since Cocso2d-x.

I didn't notice any issues at the time, but have you by any chance stepped through the code to see why the values are different? Is it happening on specific devices?

AlexandreK38 commented 1 month ago

I recall making the change to support the rounded corners, and added the call: static int* cornerRadii = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

The static int* safeInsets isn't new; it's been that way since Cocso2d-x.

I didn't notice any issues at the time, but have you by any chance stepped through the code to see why the values are different? Is it happening on specific devices?

I tried on 2 devices and same behavior. For safeInsets it's true that it was like this before your changes, and the issue was already there. On my devices I have 0, so even if the second and later calls are wrong I didn't notice.

It's your cornerRadii that made me look what the values were as they changed: First call I have 70 for the rounded corners, for all the 4 corners Second and later calls I have 0, and it's not calling the AxmolEngine.getDeviceCornerRadii.

I noticed that the cornerRadii address is still the same between calls. The fact that declaring static make the call only once to the function or initializer, so the static int* keeps the address given in return by the JNI to access the Java int*; it was valid at the first call but after the JNI address is no more valid.

As side note, I hope there isn't many calls to the JNI stored in static var with pointer in Axmol, because this could cause much troubles

rh101 commented 1 month ago

I noticed that the cornerRadii address is still the same between calls. The fact that declaring static make the call only once to the function or initializer, so the static int* keeps the address given in return by the JNI to access the Java int*; it was valid at the first call but after the JNI address is no more valid.

I'll check that out later today.

As side note, I hope there isn't many calls to the JNI stored in static var with pointer in Axmol, because this could cause much troubles

The only reason I can think of for the use of the statics is to avoid JNI calls for data that should not be changing is most likely because they're expensive to make. The pointer is pointing to an address that should not change (from my understanding), because of the use of the final int[] in the Java code etc.. Isn't that the case?

AlexandreK38 commented 1 month ago

The only reason I can think of for the use of the statics is to avoid JNI calls for data that should not be changing is most likely because they're expensive to make. The pointer is pointing to an address that should not change (from my understanding), because of the use of the final int[] in the Java code etc.. Isn't that the case?

Yeah for sure it’s to save JNI calls; I also thought that at first and tried to store as static member in AxmolEngine the int[] but no changes, but what was strange is that I got 0 after even if it could be from anywhere in the memory. Then I looked at JniHelper::callStaticIntArrayMethod and understood: https://github.com/axmolengine/axmol/blob/f320ba1f668dbb6527b1f48c60cf8b213ac01941/core/platform/android/jni/JniHelper.h#L271

The function declares a static int ret[32] and then memcopy the result got from java into this array… But any code calling this method will use the same ret array in memory as it’s static…. and the last call will override the previous result… and what is the other call right after? The safeInsets ones, that returns 0. That explains why I have always 0 after. The static jnihelper callStaticIntArrayMethod and callStaticFloatArrayMethod methods work but only once. I guess they are not used more than once, and for the Int one it was the case until your PR added a second call. Quite a dangerous behavior

I also wonder why it’s hard coded 32 as maximum array length. We can’t remove the static keyword for ret because we will return a local var address, we need another way to return the array, for example a std::vector<int>

rh101 commented 1 month ago

The function declares a static int ret[32] and then memcopy the result got from java into this array… But any code calling this method will use the same ret array in memory as it’s static….

I had absolutely no idea that this was the case, given that the method is generic. Checking back how long it's been in there, again from Cocos2d-x: https://github.com/cocos2d/cocos2d-x/blob/76903dee64046c7bfdba50790be283484b4be271/cocos/platform/android/jni/JniHelper.h#L199

I guess they are not used more than once, and for the Int one it was the case until your PR added a second call. Quite a dangerous behavior

That is exactly the reason. Thanks for looking into the issue.

A few possible ways to fix this come to mind:

  1. It returns a new std::vector each time after copying the data from the java side into it, which also means the caller can now know the true size of the data, because at the moment they cannot; the array size is assumed, which is potentially dangerous.
  2. It is left as-is, but the data from it is copied over as exactly 32 ints by the caller, but with this, the method should be renamed to reflect the hard-coded size. The caller should no longer hold a pointer, but copy the data, and handle its own caching of the data rather than the pointer.

No matter what, the code calling callStaticIntArrayMethod should be the one responsible for caching the data, not the pointer to the data.

Any other ideas on how this should be implemented?

EDIT: The safe area and corner radii are affected by the layout mode set by the application. Would the layout mode ever change at runtime (check display-cutout and rounded-corners)? If they can change, then caching the values is pointless, and we would have to make the JNI calls on every request for the values.

rh101 commented 1 month ago

Possible solution:

template <typename... Ts>
static axstd::pod_vector<int32_t> callStaticIntArrayMethod(const char* className, const char* methodName, Ts&&... xs)
{
    ax::JniMethodInfo t;
    const char* signature = jni::TypeSignature<jni::Array<jint>(std::decay_t<Ts>...)>{}();
    if (ax::JniHelper::getStaticMethodInfo(t, className, methodName, signature))
    {
        LocalRefMapType localRefs;
        jintArray array =
            (jintArray)t.env->CallStaticObjectMethod(t.classID, t.methodID, convert(localRefs, t, xs)...);

        if (array == nullptr)
        {
            t.env->DeleteLocalRef(t.classID);
            deleteLocalRefs(t.env, localRefs);
            return {};
        }

        jsize len = t.env->GetArrayLength(array);
        axstd::pod_vector<int32_t> result(len);
        jint* elems = t.env->GetIntArrayElements(array, 0);
        if (elems)
        {
            memcpy(result.data(), elems, sizeof(int32_t) * len);
            t.env->ReleaseIntArrayElements(array, elems, 0);
        };
        t.env->DeleteLocalRef(t.classID);
        deleteLocalRefs(t.env, localRefs);
        return result;
    }
    else
    {
        reportError(className, methodName, signature);
    }
    return {};
}

then in GLViewImpl-android.cpp:

static axstd::pod_vector<int32_t> cornerRadii = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

and static axstd::pod_vector<int32_t> safeInsets = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getSafeInsets");

Instead of checking for a nullptr, we simply check for the actual size we need: if (safeInsets.size() >= 4) etc. etc.

This way the caller is responsible for caching the values, as it is not the responsibility of the JNI helper methods to be caching values.

rh101 commented 1 month ago

@AlexandreK38 Check #2178 please. This is all assuming that the values of the safe area should never change for the duration of the application, and that the app layout is only set once on start-up. The functionality hasn't changed from how it is right now, except the bug is fixed.

AlexandreK38 commented 1 month ago

@rh101 I never saw axstd::pod_vector type until now but yeah it's all good for me 👍

EDIT: The safe area and corner radii are affected by the layout mode set by the application. Would the layout mode ever change at runtime (check display-cutout and rounded-corners)? If they can change, then caching the values is pointless, and we would have to make the JNI calls on every request for the values.

The cut out is changed in the AppActivity in the onCreate, but I suppose we don't change it after. I don't on my side, we can assume it is not changed, as you did.