Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by updating Gradle build scripts, adding generics to core utilities and rendering pipelines, and transitioning to newer FrameBuffer and Texture APIs. It also addresses compiler warnings and marks legacy methods as deprecated. Feedback focuses on critical API compatibility regressions in the Android modules where newer methods were introduced without version guards, potentially breaking support for older API levels. Additionally, several suggestions were provided to include @SuppressWarnings annotations on new helper methods to fully resolve unchecked cast warnings.
| Display disp = wind.getDefaultDisplay(); | ||
| Log.d("AndroidHarness", "Resolution from Window, width:" + disp.getWidth() + ", height: " + disp.getHeight()); | ||
| Point displaySize = new Point(); | ||
| disp.getSize(displaySize); |
There was a problem hiding this comment.
The method Display.getSize(Point) was introduced in API level 13. Since jme3-android supports down to API level 9 (as specified in OGLESContext), this change will cause a NoSuchMethodError on devices running Android versions older than 3.2. To maintain compatibility, you should use a version check or continue using the deprecated getWidth() and getHeight() methods for older devices.
| Drawable drawable = splashImageView.getContext().getDrawable(splashPicID); | ||
| if (drawable instanceof NinePatchDrawable) { | ||
| splashImageView.setBackgroundDrawable(drawable); | ||
| splashImageView.setBackground(drawable); |
There was a problem hiding this comment.
These changes introduce API regressions for older Android devices:
Context.getDrawable(int)(line 416) requires API level 21.View.setBackground(Drawable)(line 418) requires API level 16.
Since the engine supports API level 9, these calls will crash on older platforms. To fix the deprecation warnings while preserving compatibility, use Build.VERSION.SDK_INT checks to call the appropriate methods based on the platform version.
| private static <T> WeakReference<T>[] newWeakReferenceArray(int size) { | ||
| WeakReference<T>[] refs = (WeakReference<T>[]) new WeakReference<?>[size]; | ||
| return refs; | ||
| } |
There was a problem hiding this comment.
The cast to WeakReference<T>[] on line 49 is unchecked and will generate a compiler warning. Since the purpose of this PR is to reduce warning noise, this helper method should be annotated with @SuppressWarnings("unchecked").
| private static <T> WeakReference<T>[] newWeakReferenceArray(int size) { | |
| WeakReference<T>[] refs = (WeakReference<T>[]) new WeakReference<?>[size]; | |
| return refs; | |
| } | |
| @SuppressWarnings("unchecked") | |
| private static <T> WeakReference<T>[] newWeakReferenceArray(int size) { | |
| return (WeakReference<T>[]) new WeakReference<?>[size]; | |
| } |
| private static <T> Entry<T>[] newTable(int size) { | ||
| Entry<T>[] table = (Entry<T>[]) new Entry<?>[size]; | ||
| return table; | ||
| } |
There was a problem hiding this comment.
The cast to Entry<T>[] on line 301 will trigger an unchecked cast warning. To fully achieve the goal of reducing warning noise, please add @SuppressWarnings("unchecked") to this helper method.
| private static <T> Entry<T>[] newTable(int size) { | |
| Entry<T>[] table = (Entry<T>[]) new Entry<?>[size]; | |
| return table; | |
| } | |
| @SuppressWarnings("unchecked") | |
| private static <T> Entry<T>[] newTable(int size) { | |
| return (Entry<T>[]) new Entry<?>[size]; | |
| } |
| public Entry<String, JsonElement>[] entrySet() { | ||
| Set<Entry<String, com.google.gson.JsonElement>> entrySet = obj().entrySet(); | ||
| Entry<String, JsonElement>[] entries = new Entry[entrySet.size()]; | ||
| Entry<String, JsonElement>[] entries = (Entry<String, JsonElement>[]) new Entry<?, ?>[entrySet.size()]; |
There was a problem hiding this comment.
This generic array creation and cast will result in an unchecked warning. It is recommended to add @SuppressWarnings("unchecked") to the method or the assignment to silence the warning, consistent with the PR's objective.
| Entry<String, JsonElement>[] entries = (Entry<String, JsonElement>[]) new Entry<?, ?>[entrySet.size()]; | |
| @SuppressWarnings("unchecked") | |
| Entry<String, JsonElement>[] entries = (Entry<String, JsonElement>[]) new Entry<?, ?>[entrySet.size()]; |
8Keep
force-pushed
the
warning-fixes
branch
2 times, most recently
from
c7260b8 to
8b7d21a
Compare
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
1 participant
testing, do not look... did not mean to create a real pr here yet