Repository navigation
Run Unity 2D projects as Codename One applications - #5976
shai-almog wants to merge 34 commits into
Conversation
A Unity project's Assets and ProjectSettings in common/src/main/unity build unmodified: the .NET SDK compiles the C# scripts, maven/cil-translator turns the assembly into Java class files and compiles every scene, prefab, animator controller, clip, tile map and particle system to reflection-free Java, and maven/unity-compat is the runtime they run against (a .NET base library, the UnityEngine API over com.codename1.gaming, and our own TextMesh Pro and Cinemachine implementations). Maven (compile-unity, import-unity-project) and Gradle (compileUnity) share UnityProjectBuilder. Both modules sit behind the unity-compat profile and are published by the release. Float results have to match C# on every target, so native builds get -ffp-contract=off and the JavaScript target rounds float results with Math.fround. Bugs this uncovered and fixes outside the new modules, each with a test: - ParparVM JavaScript: wide stack shuffles, MULTIANEWARRAY dimension pops, a call result compared with FCMP/DCMP evaluated more than once, SWAP losing the lower value. - ParparVM native: arraycopy/clone of an array of primitive arrays took the primitive path. - GameView: a key repeat released a held key; a focused hosted child kept the multi-touch arrays from the view. - Box2D: chain against chain threw in World.popContact; the broad phase allocated a merge buffer on every step above 31 pairs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
|
Compared 193 screenshots: 193 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 178 screenshots: 178 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Cloudflare Preview
|
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
|
Compared 193 screenshots: 193 matched. Benchmark ResultsDetailed Performance Metrics
ParparVM vs HotSpot (JDK 25): Windows x64Runner CPU: AMD64 Family 25 Model 17 Stepping 1, AuthenticAMD (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
Compared 193 screenshots: 193 matched. ParparVM vs HotSpot (JDK 25): Linux x64Runner CPU: AMD EPYC 9V74 80-Core Processor (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
Compared 193 screenshots: 193 matched. ParparVM vs HotSpot (JDK 25): Linux arm64Runner CPU: Neoverse-N2 (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
Compared 193 screenshots: 193 matched. Benchmark ResultsDetailed Performance Metrics
ParparVM vs HotSpot (JDK 25): Windows arm64Runner CPU: ARMv8 (64-bit) Family 8 Model D49 Revision 0, MICROSOFT CORPORATION (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5639acb341
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| UnityRuntime.begin(); | ||
|
|
||
| view = new UnityGameView(); |
There was a problem hiding this comment.
Calling UnityRuntime.begin() before the UnityGameView is created and initialized runs every script's Awake/OnEnable while PlayerPrefs still uses its transient in-memory fallback and AudioSource has no output. Consequently, saved preferences read in Awake appear missing, writes made there disappear once the store is installed, and every scene AudioSource with Play On Awake marks itself playing without ever sending audio to the device. Initialize these view-backed services before waking the first scene.
Useful? React with 👍 / 👎.
| if (negative) { | ||
| value = -value; |
There was a problem hiding this comment.
For the valid input "-2147483648", negating the scanned magnitude produces Long.MIN_VALUE, which is also used as the invalid-input sentinel. Parse therefore throws FormatException, and TryParse returns false, instead of producing Integer.MIN_VALUE. Use a distinct parse-status indicator so the minimum Int32 remains representable.
Useful? React with 👍 / 👎.
| c.resumeAt = 0f; | ||
| c.phase = 0; |
There was a problem hiding this comment.
When StartCoroutine is called from Start or Update, it immediately advances the iterator and adds it with phase == 0 and resumeAt == 0; the same frame's later runCoroutines(0) then advances it again. Thus a first yield return null resumes in the current frame rather than the next frame as promised, so initialization or frame-based loops can execute twice in one frame. Record the frame at which a null/default yield may next resume.
Useful? React with 👍 / 👎.
| public static float get_timeSinceLevelLoad() { | ||
| return time; |
There was a problem hiding this comment.
get_timeSinceLevelLoad() returns the application-wide Time.time, but changeScene() never resets that clock. After loading a second scene, scripts therefore see time since application startup rather than time since the current scene was loaded, which breaks scene-local timers and spawn schedules. Track a separate scene-load timestamp or reset a dedicated level clock on every load.
Useful? React with 👍 / 👎.
| public static boolean get_isMobilePlatform() { | ||
| return false; |
There was a problem hiding this comment.
Application.isMobilePlatform is hard-coded to false, including when the generated application runs on Android or iOS. Unity scripts commonly use this property to enable touch controls, mobile layouts, or device-specific behavior, so those paths remain disabled on the very device builds this compatibility layer targets. Have the host install or derive the actual platform value instead of returning a constant.
Useful? React with 👍 / 👎.
| xvfb-run -a mvn deploy -Psign-artifacts -Dgpg.passphrase=$MAVEN_GPG_PASSPHRASE \ | ||
| -Plocal-dev-javase -Darchetype.test.skip=true -DskipTests \ | ||
| -Plocal-dev-javase -Dunity-compat -Darchetype.test.skip=true -DskipTests \ |
There was a problem hiding this comment.
The release step first runs update-version.sh, whose mvn versions:set invocation does not activate unity-compat, and only activates this profile in the later deploy command. These two modules are therefore absent from the version-update reactor and retain their explicit 8.0-SNAPSHOT parent and artifact versions; once the root POM has the tag version, activating them for deploy produces a parent-version mismatch or publishes under snapshot coordinates instead of the tag. Run the version update with this profile active or explicitly update these two POMs before deployment.
Useful? React with 👍 / 👎.
| mv.visitJumpInsn(IFNE, ok); | ||
| invoke(INVOKESTATIC, Names.INTEROP, "divideByZero", "()V", false); |
There was a problem hiding this comment.
The generated check covers only a zero divisor. For int.MinValue / -1 and long.MinValue / -1, CIL signed division must throw OverflowException, while JVM IDIV/LDIV silently returns the minimum value. Translated C# therefore continues with a corrupt arithmetic result and cannot catch the expected exception; add the minimum-value/negative-one check before emitting the division opcode.
Useful? React with 👍 / 👎.
| public static int Abs(int v) { | ||
| return v < 0 ? -v : v; |
There was a problem hiding this comment.
System.Math.Abs(int.MinValue) must throw OverflowException, because the positive magnitude is not representable, but negating it here wraps and returns the same negative value; the long overload has the same defect. Scripts using the documented System.Math surface can therefore propagate a negative value from an operation guaranteed to be nonnegative. Detect the minimum values and throw the runtime's OverflowException before negating.
Useful? React with 👍 / 👎.
… perf baseline On the Ubuntu runner dotnet is linked into /usr/bin, so dropping every PATH directory that holds it also dropped env, bash and mvn. The directory is now shadowed by links to everything in it except dotnet. objectAllocation RAM on Neoverse-N2 is bimodal; the row is recalibrated from this run and three master runs with a wider RAM tolerance. No improvement is claimed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review findings (rationale for the ones left alone is in comments at the code concerned): - Install the view's services (PlayerPrefs store, audio, platform) before the first scene wakes. - A coroutine started from Start or Update resumes its first null yield on the next frame. - Time.timeSinceLevelLoad restarts on a scene load. - Application.platform and isMobilePlatform come from the host platform. - Signed division and remainder of MinValue by -1 throw OverflowException, as does System.Math.Abs of MinValue. JavaScript screenshots: XYChart rotated the canvas and rotated it back, which with float-rounded angles leaves a transform 1e-14 off identity, and the JavaScript port draws DOM text only for an exact identity. XYChart and ChartComponent now restore the transform they were handed, and the port's DOM text decision tolerates that residue. A differential fixture against the JVM found four older JavaScript target bugs: int to float/double read an un-normalized int, float/double to int wrapped instead of saturating, a static read before its first write was undefined, and Float.toString printed the double. The offscreen check now runs on every scene sample in the integration test. It found: a flick release delivered at the press point because CodenameOneImplementation aliased the shared pointer arrays, GameView missing single finger presses after the first drag, and a sub-image cache keyed by corner only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…e numeric fixture The JavaScript target now truncates a float product the way the JVM does, so the six theme screens that size from one move by a pixel; the goldens are this run's CI captures. JsNumericDifferentialApp compares narrowing compound assignments against the JVM on purpose and joins the other compiler fixtures CodeQL does not read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Compared 214 screenshots: 214 matched. |
…lLoad Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Compared 171 screenshots: 171 matched. |
|
Compared 175 screenshots: 175 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 236 screenshots: 236 matched. |
|
@codex review |
|
Compared 176 screenshots: 176 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a51fcb4b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public static boolean Any(IEnumerable_1 source, Class t) { | ||
| return walk(source).MoveNext(); |
There was a problem hiding this comment.
When the source is a custom enumerator or a compiler-generated iterator with a finally block, terminal operations that stop early—such as this Any, First, Contains, ElementAt, and Take—abandon the enumerator without calling Dispose(), even though IEnumerator_1 implements IDisposable. The iterator's cleanup therefore never runs after an early result or predicate exception; wrap enumeration in try/finally and dispose the enumerator on every exit path.
Useful? React with 👍 / 👎.
| public static Object GetValues(Type enumType) { | ||
| Type t = enumType(enumType); | ||
| int[] out = new int[t.values.length]; | ||
| for (int i = 0; i < out.length; i++) { | ||
| out[i] = (int) t.values[i]; |
There was a problem hiding this comment.
For any enum whose underlying type is not int—for example, enum E : long { Huge = 1L << 40 }—Enum.GetValues always returns an int[] and truncates every member. Translated callers cast the result to the enum's normalized JVM array type (for this example, long[]), so the usual (E[])Enum.GetValues(typeof(E)) either fails with a cast error or receives corrupted values. Construct the primitive array corresponding to the enum's recorded underlying type instead.
Useful? React with 👍 / 👎.
| for (int i = 1; i < out.length; i++) { | ||
| int v = out[i]; | ||
| int j = i - 1; | ||
| while (j >= 0 && out[j] > v) { | ||
| out[j + 1] = out[j]; |
There was a problem hiding this comment.
When an enum contains negative members, such as the common flags value All = -1, this signed comparison places that member before zero. Enum.GetValues orders members by the unsigned magnitude of their binary representation, so { All = -1, None = 0, One = 1 } must enumerate as None, One, All, whereas this code returns All, None, One. Use an unsigned comparison appropriate to the enum's underlying width.
Useful? React with 👍 / 👎.
| } else if (c == '}') { | ||
| if (i + 1 < n && format.charAt(i + 1) == '}') { | ||
| i++; | ||
| } | ||
| sb.append('}'); |
There was a problem hiding this comment.
For a malformed composite format such as "value }", .NET throws FormatException, but this branch appends the unmatched brace and returns normally. This changes control flow for scripts that validate or recover from user-provided format strings; only a doubled }} should emit a literal closing brace, while a standalone } must be rejected.
Useful? React with 👍 / 👎.
| public static Type $of(Class type, String fullName) { | ||
| Type t = (Type) byClass.get(type); | ||
| if (t == null) { | ||
| t = new Type(fullName, type, null, null, false); | ||
| byClass.put(type, t); |
There was a problem hiding this comment.
The cache key is only the JVM Class, but multiple distinct .NET primitives share one box class: int and uint both use Integer.class, long and ulong use Long.class, and byte and sbyte use Byte.class. Consequently, whichever type is requested first is returned for both, making typeof(int) == typeof(uint) true and potentially reporting the wrong FullName. Include the .NET type identity in the cache key rather than keying solely by the erased Java class.
Useful? React with 👍 / 👎.
| static long number(Object value) { | ||
| if (value instanceof EnumBox) { | ||
| return ((EnumBox) value).value; |
There was a problem hiding this comment.
This conversion discards the EnumBox.type, so APIs such as Enum.IsDefined, Enum.GetName, HasFlag, and CompareTo accept a boxed member of an unrelated enum and operate only on its numeric value. For example, firstEnum.HasFlag(secondEnum) can return true when their bits overlap, whereas .NET throws ArgumentException for differing enum types. Validate the box's enum type against the requested or receiver type before extracting its number.
Useful? React with 👍 / 👎.
| public void set_time(float value) { | ||
| startedAt = Time.unscaledTime - value; | ||
| } |
There was a problem hiding this comment.
When a playing source executes audio.time = value, this setter only changes the runtime's bookkeeping and never seeks the device's Media, so the audible clip continues from its old position while scripts observe a different position. The bookkeeping is also immediately wrong when pitch != 1, because get_time() multiplies elapsed time by pitch but this setter does not divide the requested clip position by pitch. Add a seek operation to AudioOutput and derive startedAt consistently with the playback rate.
Useful? React with 👍 / 👎.
| public static String Trim(String s) { | ||
| return s.trim(); | ||
| } |
There was a problem hiding this comment.
String.trim() only removes characters at or below U+0020, whereas System.String.Trim() removes Unicode whitespace. Inputs surrounded by characters such as non-breaking space (U+00A0) or em space (U+2003) therefore remain untrimmed, even though this runtime already recognizes them in Char_.IsWhiteSpace. Scan the ends using the .NET whitespace predicate instead of delegating to Java's trim().
Useful? React with 👍 / 👎.
| String wanted = value.trim(); | ||
| for (int i = 0; i < t.names.length; i++) { | ||
| if (ignoreCase ? t.names[i].equalsIgnoreCase(wanted) : t.names[i].equals(wanted)) { | ||
| return new EnumBox(t, t.values[i]); | ||
| } | ||
| } | ||
| try { | ||
| return new EnumBox(t, Long.parseLong(wanted)); |
There was a problem hiding this comment.
Enum.Parse accepts a comma-separated list of member names, which is the standard way to parse flags such as "Read, Write"; this implementation only checks the entire string against one member and then attempts numeric parsing. Valid combined values therefore throw ArgumentException. Split on commas, trim each name, resolve each with the requested case sensitivity, and combine their underlying values.
Useful? React with 👍 / 👎.
| if (t instanceof NegativeArraySizeException) { | ||
| return new OverflowException(); | ||
| } | ||
| return t; |
There was a problem hiding this comment.
Several public runtime APIs throw java.lang.IllegalArgumentException for invalid C# calls, including PlayerPrefs, SceneManager, Input, physics queries, and Instantiate, but this dispatcher leaves that throwable unchanged. Because generated catch clauses test against the custom System.Exception hierarchy, even catch (Exception) cannot catch these failures and the raw JVM exception escapes the script. Throw the corresponding compatibility exception at each API or map Java argument exceptions here.
Useful? React with 👍 / 👎.
|
Compared 176 screenshots: 176 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…seline Base library: LINQ operators dispose their enumerators on every exit, Enum values keep their underlying type and unsigned order, Enum.Parse takes comma lists, HasFlag/CompareTo/IsDefined refuse another enum's member, String.Format rejects a lone closing brace, Trim follows Char.IsWhiteSpace, typeof(int) and typeof(uint) are distinct, a float key's two zeros are one key, and a Java exception thrown by the runtime is caught by a script's catch (Exception). AudioSource.time seeks the device's media and accounts for pitch. The macOS draw-gradient golden is this branch's CI capture: with -ffp-contract=off the mutable-image ellipse edge now equals the on-screen one. Linux x64 perf baseline rows recalibrated per runner CPU from PR and master runs; the shift is present on master at the base commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0abf10218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Image image = (Image) paintedTexts.get(key); | ||
| if (image == null) { | ||
| image = paint(d, width, height); | ||
| paintedTexts.put(key, image); |
There was a problem hiding this comment.
When a Text component displays a changing value such as a timer, score, or frame counter, every distinct string creates an Image and stores it permanently in paintedTexts; the map is never cleared or evicted. A frequently updated label therefore retains thousands of raster images and can exhaust device memory during an otherwise normal game session. Keep only current/recent entries or use a bounded cache.
Useful? React with 👍 / 👎.
| if (yielded instanceof WaitForSeconds) { | ||
| c.resumeAt = Time.time + ((WaitForSeconds) yielded).seconds; | ||
| } else if (yielded instanceof Coroutine) { | ||
| c.waitingFor = (Coroutine) yielded; | ||
| } else if (yielded instanceof WaitForFixedUpdate) { |
There was a problem hiding this comment.
When a coroutine uses the common yield return ChildRoutine() form, the yielded value is an IEnumerator, but this chain recognizes only a started Coroutine; it consequently treats the child iterator like null, never executes it, and resumes the parent on the next frame. Add nested-enumerator handling so the parent waits while the yielded iterator is driven to completion.
Useful? React with 👍 / 👎.
| public static float get_unscaledDeltaTime() { | ||
| return unscaledDeltaTime; |
There was a problem hiding this comment.
When Time.unscaledDeltaTime is read from FixedUpdate, Unity reports the unscaled duration of that fixed step, but this getter always returns the enclosing render frame's dt. At nonmatching render/physics rates—or when several fixed steps run in one frame—fixed-update timers and movement therefore advance by the render delta once per physics step, producing speed and scheduling errors.
Useful? React with 👍 / 👎.
| if (f.isDirectory()) { | ||
| collectStamped(f, prefix + f.getName() + "/", out); | ||
| } else { | ||
| out.add(prefix + f.getName() + ":" + f.length() + ":" + f.lastModified()); |
There was a problem hiding this comment.
In the new Unity incremental build cache, a source or asset rewritten with the same byte length and preserved modification time—such as after restoring or synchronizing files with timestamps intact—produces the same stamp even if its contents changed. run() then accepts state.txt and skips compilation, leaving stale translated classes or resources in the application; hash the file bytes rather than only name, size, and timestamp.
Useful? React with 👍 / 👎.
| public List_1() { | ||
| items = new Object[4]; | ||
| } | ||
|
|
||
| public List_1(int capacity) { | ||
| items = new Object[capacity < 4 ? 4 : capacity]; |
There was a problem hiding this comment.
When valid C# uses new List<T>(source)—including the new List<int>(numbers) array case explicitly anticipated by ArrayView—the translator emits a call to the erased List_1(IEnumerable_1) constructor, but this runtime class exposes only the parameterless and capacity constructors. The Unity project therefore fails translation with a missing runtime method instead of constructing the list; add the enumerable constructor and dispose its enumerator after copying.
Useful? React with 👍 / 👎.
| String simple = meta.getName().substring(0, meta.getName().length() - ".cs.meta".length()); | ||
| for (CilAssembly assembly : scripts) { | ||
| for (TypeDef t : assembly.types()) { | ||
| String full = t.fullName(); | ||
| if (full.equals(simple) || full.endsWith("." + simple)) { | ||
| scriptsByGuid.put(guid, t); |
There was a problem hiding this comment.
When a valid Unity project contains two scripts with the same filename in different folders and namespaces, each .cs.meta GUID is mapped by simple class name and this loop always selects the first matching TypeDef. Both GUIDs can therefore resolve to the same script class, causing scene objects to receive the wrong behaviour and serialized fields from the other script to be dropped. Use source/PDB information or another unambiguous file-to-type mapping.
Useful? React with 👍 / 👎.
| GameObject go = (GameObject) objects.get(i); | ||
| if (go.destroyed || !go.activeInHierarchy()) { | ||
| continue; | ||
| } | ||
| if ((name != null && !go.name.equals(name)) || (tag != null && !go.tag.equals(tag))) { | ||
| continue; |
There was a problem hiding this comment.
When a script calls GameObject.Find("Canvas/Panel/Button"), Unity interprets the slash-separated argument as a hierarchy path, but this implementation compares the whole argument only against each object's leaf name. Such lookups always return null unless an object literally contains slashes in its name, breaking a standard way scripts locate nested scene objects; resolve the root and traverse children as the existing Transform.Find implementation does.
Useful? React with 👍 / 👎.
| public java.lang.Object GetComponentInChildren(Class type) { | ||
| if (!active) { | ||
| return null; | ||
| } | ||
| java.lang.Object c = GetComponent(type); |
There was a problem hiding this comment.
When GetComponentInChildren is invoked on an inactive GameObject, Unity still searches that root object even though the one-argument overload excludes its inactive descendants, but this early return skips the root as well. Managers querying an inactive prefab or temporarily disabled object therefore receive null despite the requested component being attached directly to it; GetComponentsInChildren has the same issue through collect, so both traversals should always inspect their starting object.
Useful? React with 👍 / 👎.
| public static boolean IsDigit(char c) { | ||
| return c >= '0' && c <= '9'; | ||
| } |
There was a problem hiding this comment.
For decimal digits outside ASCII, such as Arabic-Indic ٣ or full-width ５, System.Char.IsDigit must return true because they have the Unicode decimal-digit category, but this implementation accepts only 0 through 9. Translated validation and parsing code therefore rejects digit strings that work in .NET; use a Unicode digit-category check rather than an ASCII range.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86a52fd46d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cw.visitField(ACC_PUBLIC | (f.isStatic() ? ACC_STATIC : 0), fieldName, names.descriptor(f.type), null, | ||
| null).visitEnd(); |
There was a problem hiding this comment.
Because all constructed generic types are erased to one JVM class, emitting an ordinary Java static field here makes instantiations such as Cache<int> and Cache<string> share the same storage and static initializer. Writing Cache<int>.Value can therefore change Cache<string>.Value, silently corrupting common generic singleton, cache, and registry patterns; specialize static storage by closed type or reject generic types with static state rather than sharing it.
Useful? React with 👍 / 👎.
| public static String GetName(Type enumType, Object value) { | ||
| Type t = enumType(enumType); | ||
| int at = t.indexOf(t.held(number(value))); | ||
| return at < 0 ? null : t.names[at]; |
There was a problem hiding this comment.
When value is another enum type or an integer of a different width, this unconditionally extracts and truncates its number, so calls such as Enum.GetName(typeof(ByteEnum), (int)1) can return a member instead of throwing. .NET requires the value to be the requested enum type or exactly its underlying type, otherwise it raises ArgumentException, so translated exception handling and validation diverge; apply the same type checks already used by IsDefined before looking up the number. See the Enum.GetName contract.
Useful? React with 👍 / 👎.
| if (yielded instanceof WaitForSeconds) { | ||
| c.resumeAt = Time.time + ((WaitForSeconds) yielded).seconds; |
There was a problem hiding this comment.
When WaitForSeconds is yielded from Start, FixedUpdate, or a collision callback, Time.time has not yet been advanced for the current frame, so this deadline counts the current frame's duration toward the wait. A coroutine started in Start during a 0.1-second frame with a one-second wait can therefore resume after only about 0.9 seconds following that frame, and multiple fixed steps make the error larger. Unity starts this wait at the end of the current frame, so defer establishing the deadline until then or account for the pending frame time. See the WaitForSeconds timing contract.
Useful? React with 👍 / 👎.
| if (playing && clip != null) { | ||
| startedFrom = position(); | ||
| startedAt = paused ? pausedAt : now(); | ||
| } | ||
| pitch = value; |
There was a problem hiding this comment.
When a script changes source.pitch after Play(), this updates only the runtime's playback clock; AudioOutput is never notified, so the audible voice continues at its original pitch while time, looping, and expiration are calculated at the new speed. Games that animate pitch therefore hear no change and can see isPlaying become false before or after the sound actually ends. Add an output operation that updates the active voice's playback rate when this setter runs, as required by Unity's AudioSource.pitch behavior.
Useful? React with 👍 / 👎.
| lastMouseY = mouseY; | ||
| // GetAxis eases towards the key's value at Unity's default rate of | ||
| // three units a second, and snaps through zero on a reversal. | ||
| float step = 3f * dt; |
There was a problem hiding this comment.
When a key-driven axis in InputManager.asset uses non-default gravity or sensitivity, the generated axis retains only its key mappings and this hard-coded step makes both acceleration and recentering run at three units per second. Input.GetAxis therefore responds at the wrong rate in projects that tune those fields, affecting movement and control feel; carry each axis's configured gravity and sensitivity into the runtime instead. The Unity Input Manager manual defines these fields as the rates toward the target and neutral values.
Useful? React with 👍 / 👎.
| if (clip.loop) { | ||
| float b = (from - at) / length; | ||
| float a = (until - at) / length; | ||
| due = floor(a) > floor(b) || (from == 0f && at == 0f); |
There was a problem hiding this comment.
When one animator tick spans more than one loop because of a short clip, high animator speed, or a long frame, this boolean only records that at least one event boundary was crossed and invokes the event once. The additional loop occurrences are lost even though Unity processes the event once per animation loop, so event-driven attacks, sounds, and counters under-fire during frame stalls; count the crossed boundaries and invoke the event for each one. See Unity's AnimationEvent documentation.
Useful? React with 👍 / 👎.
…, events Review findings: - A static field of a generic type was one Java static for every instantiation, so Singleton<GameManager> and Singleton<AudioManager> shared their instance. Each constructed type now gets its own static context (GenericStatics): a subclass per closed type whose class initializer runs the type's static constructor once, as .NET does, passed to static methods as a hidden last parameter and kept by instances. Forms that cannot be keyed fail the build naming the site. - Time.time advances at the start of the frame, so a WaitForSeconds, Invoke or delayed Destroy begun in Start or a fixed step no longer counts the frame it was started in. - Enum.GetName compares the value at full width instead of truncating it to the enum's storage. - Key axes take gravity, sensitivity and snap from InputManager.asset. - An animation event fires once per loop a tick crosses, in order. - AudioSource stops every voice it started; a pitch the device cannot apply is a build note. Found on the way: `instance == null` inside a generic type constrained to a class failed bytecode verification. The macOS translator RAM row is rebaselined from this PR's runs and four master controls: master measured 0.68 and 0.55 on two runs of one commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01ce0c0887
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (erased && declared.kind == CilType.Kind.SZARRAY && Names.isTypeVariable(declared.element) | ||
| && s.kind == CilType.Kind.SZARRAY && names.norm(s.element).isPrimitive()) { |
There was a problem hiding this comment.
When translated code calls an exposed generic member such as new List<int> { 1 }.CopyTo(new int[1]), this condition unconditionally raises TranslationException, so an ordinary supported List<T> operation prevents the Unity project from building. Add a primitive-array bridge that copies/unboxes the erased elements into the caller's array, analogous to the conversion already performed for generic array return values.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| public static String New(char c, int count) { | ||
| StringBuilder sb = new StringBuilder(count); |
There was a problem hiding this comment.
When translated code executes new string(c, count) with a negative runtime count, StringBuilder throws NegativeArraySizeException, which Interop.wrap converts to OverflowException; .NET instead requires ArgumentOutOfRangeException for this constructor. Validate count before allocating so translated catch blocks observe the expected exception.
Useful? React with 👍 / 👎.
| if (looping.containsKey(key)) { | ||
| play(key.intValue(), clip, volume, 1f, true); |
There was a problem hiding this comment.
When a looped source changes volume or mute after playback begins, AudioSource updates the current voice through output.volume, but this restart uses the original volume captured by the first play call. At the next loop boundary the sound therefore jumps back to its initial level (and a muted source can become audible again); retain the current per-voice volume and use it when restarting the media.
Useful? React with 👍 / 👎.
| if (p.contact == contact) { // NOPMD CompareObjectsWithEquals | ||
| p.contact = null; | ||
| } |
There was a problem hiding this comment.
When two colliders have multiple simultaneous fixture contacts, as with a decomposed polygon or composite collider, ending the contact currently stored in p.contact clears it even if p.count shows that another contact remains. Subsequent OnCollisionStay2D callbacks are built with a null contact and therefore report zero contact points and no normal while the colliders are still touching, which can break grounding and collision-response scripts; retain or select one of the surviving contacts until the pair fully separates.
Useful? React with 👍 / 👎.
Review findings: - A primitive array passed where a generic member takes T[] (List<int>.CopyTo(int[])) failed the build. It is passed as an array of the right boxes and copied back after the call; List<T>.CopyTo gains its four-argument overload. A constructor now runs the after-call steps a method does. - new string(c, negative) throws ArgumentOutOfRangeException, and the string members around it throw the exception types .NET does. - A looped sound keeps its current volume, and its mute, across the restart at each loop. - A collider pair touching through several contacts keeps reporting one of the survivors to OnCollisionStay2D until it separates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 002982eac3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return minInclusive; | ||
| } | ||
| long r = next() & 0xffffffffL; | ||
| return (int) (lo + r % span); |
There was a problem hiding this comment.
When the requested span does not divide 2^32, reducing one unsigned 32-bit sample with % span makes some results more likely than others. For example, with Range(0, 1500000000), values below 1294967296 have three possible source samples while the remaining values have only two, so individual low values are 50% more likely; this can visibly skew procedural selection. Use rejection sampling or another unbiased bounded-integer method.
Useful? React with 👍 / 👎.
| if (a instanceof Integer) { | ||
| return Int32_.CompareTo(((Integer) a).intValue(), ((Integer) b).intValue()); |
There was a problem hiding this comment.
When translated code invokes non-generic IComparable.CompareTo with a non-null value of the wrong runtime type, such as ((IComparable)1).CompareTo("x"), these casts throw Java ClassCastException; Interop.wrap then exposes that as InvalidCastException. The .NET primitive and string implementations instead throw ArgumentException, so translated catch clauses take the wrong path. Validate that b has the same boxed type before dispatching.
Useful? React with 👍 / 👎.
| if (b == null) { | ||
| return 1; |
There was a problem hiding this comment.
When both operands are null, as in IComparable value = null; value.CompareTo(null), MethodTranslator rewrites the interface call to this static helper and this early return answers 1 before the receiver check at the end. An instance call on a null receiver must throw NullReferenceException; check a before applying the rule that a valid comparable sorts after a null argument.
Useful? React with 👍 / 👎.
| || name.equals("System.Collections.Generic.ICollection`1") | ||
| || name.equals("System.Collections.Generic.IList`1") | ||
| || name.equals("System.Collections.Generic.IReadOnlyCollection`1") | ||
| || name.equals("System.Collections.Generic.IReadOnlyList`1") |
There was a problem hiding this comment.
When an array is assigned or passed as ICollection<T> (for example, ICollection<int> values = new int[3]), this branch deliberately replaces it with ArrayView, but ArrayView implements only IEnumerable_1. A subsequent values.Count call is therefore emitted as an ICollection_1 interface invocation on an object that does not implement that interface and fails at runtime with IncompatibleClassChangeError; IList<T> and the read-only interfaces listed here have the same problem. Either make the view implement the advertised interfaces with array-appropriate behavior or do not claim these conversions are supported.
Useful? React with 👍 / 👎.
| // that keys overlap, the display takes the release of any key but | ||
| // the last two pressed for a stray one and drops it, and the key | ||
| // it belonged to stays down for good. | ||
| Display.getInstance().setMultiKeyMode(true); |
There was a problem hiding this comment.
When a UnityGameView is embedded in a larger Codename One application, initializing it unconditionally changes Display's process-wide multiKeyMode, and deinitialize() never restores the previous value. After navigating away, ordinary/native UI therefore continues using the alternate key-release behavior that Display explicitly does not enable by default because it complicates native navigation. Record whether this view enabled the mode and restore it on detach, without disabling a mode that was already owned by the host.
Useful? React with 👍 / 👎.
| if (now != null && recorded != null && now.digest.equals(recorded.digest) && staged.isDirectory() | ||
| && appImpl.isFile()) { |
There was a problem hiding this comment.
When a partial build cache retains state.txt, the staged classes, and UnityAppImpl.java but loses target/unity/resources or target/unity/runtime, this condition treats the cache as complete. A missing resources directory makes install() remove previously installed images, while a missing runtime directory prevents it from filtering runtime-owned value classes out of the staged output, so the resulting package can lack assets or contain duplicate runtime definitions. Require both auxiliary directories to exist before taking the unchanged-project path.
Useful? React with 👍 / 👎.
| while (t != null) { | ||
| java.lang.Object c = t.gameObject.GetComponent(type); |
There was a problem hiding this comment.
When an inactive parent has the requested component, this unconditional walk returns it from the no-argument GetComponentInParent<T>(). Unity's default overload searches the object itself regardless of state but includes only active parent GameObjects; inactive parents are included only by the separate includeInactive overload, as specified by Unity's API contract. Skip inactive ancestors here so scripts do not bind to disabled hierarchy branches unexpectedly.
Useful? React with 👍 / 👎.
Review findings: - Random.Range over integers redraws a sample past the last whole multiple of the span instead of reducing it with a biased modulo. Small spans almost never redraw, so seeded sequences are unchanged. - IComparable.CompareTo throws NullReferenceException for a null receiver before looking at its argument, and ArgumentException for an argument of another type, as .NET does. - An array viewed as ICollection<T> implements it (Count; Clear throws NotSupportedException, as an array does). - UnityGameView restores Display's multi-key mode when the last view goes, if it was the views that turned it on. - The unchanged-project fast path requires every part of the output it would otherwise write, so a partial cache is compiled again. - GetComponentInParent skips inactive ancestors; the object itself is always searched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e85e5a6440
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public static boolean Equals(String a, String b) { | ||
| return op_Equality(a, b); |
There was a problem hiding this comment.
When a null variable statically typed as string calls value.Equals((string)null), the translator routes the instance overload and static string.Equals(a, b) through this same helper, so this returns true instead of throwing NullReferenceException. The translator already gives object.Equals instance calls a separate $equals helper for this distinction; string instance calls need the same receiver check without changing the null-tolerant static overload.
Useful? React with 👍 / 👎.
| public static void set_volume(float value) { | ||
| AudioSource.listenerVolume = value < 0f ? 0f : value > 1f ? 1f : value; |
There was a problem hiding this comment.
When music or a one-shot is already playing and a script changes AudioListener.volume, this setter only updates the scalar used by future play calls; it never updates the active output voices, so mute/master-volume sliders leave existing sounds at their old loudness until they restart. Unity defines this property as controlling the game sound volume, so the output needs a global-volume operation or all live voices must be updated immediately. See Unity's AudioListener.volume documentation.
Useful? React with 👍 / 👎.
| @Override | ||
| protected boolean $sameType(Delegate other) { | ||
| return other instanceof Action_1; | ||
| } |
There was a problem hiding this comment.
When code calls Delegate.Combine or Delegate.Remove with delegates such as Action<int> and Action<string>, both values are erased to Action_1, so this check incorrectly treats them as the same delegate type. Combine consequently returns a multicast delegate instead of throwing ArgumentException, and invoking it can pass an Integer to the string handler and fail later with an unrelated cast error. Preserve the closed type arguments in the delegate identity; .NET requires the delegate types to match exactly, even when variance makes them assignment-compatible.
Useful? React with 👍 / 👎.
| if ("1".equals(type)) { | ||
| line(sb, RUNTIME + "Input.$mouseAxis(" + string(text(axis.get("m_Name"))) + ", (int) " | ||
| + integer(axis.get("axis"), "0") + "L, " + f(axis.get("sensitivity"), "0.1") + ");"); |
There was a problem hiding this comment.
When a project sets invert: 1 on a mouse axis such as Mouse Y, this generated call carries only the axis number and sensitivity, and the runtime Axis has no inversion state, so both Input.GetAxis and GetAxisRaw retain the original sign. Camera and aiming controls configured as inverted in Unity therefore move in the opposite direction in the translated application; propagate the setting and negate the raw axis value. Unity's Input Manager documentation specifies that Invert swaps positive and negative axis values.
Useful? React with 👍 / 👎.
| public void ClearDepth() { | ||
| minDepth = Float.NEGATIVE_INFINITY; | ||
| maxDepth = Float.POSITIVE_INFINITY; | ||
| useDepth = false; |
There was a problem hiding this comment.
When a script calls ClearDepth to temporarily disable filtering and later sets useDepth = true, these assignments replace its configured range with unbounded values, so the re-enabled query unexpectedly matches all depths. Unity specifies that ClearDepth only turns off useDepth and leaves minDepth and maxDepth unchanged; ClearNormalAngle has the same defect for its associated angles. Preserve the configured bounds and only clear the enable flags. See Unity's ContactFilter2D contract.
Useful? React with 👍 / 👎.
…s, axis invert - An instance string Equals/CompareTo on a null receiver throws NullReferenceException; static string.Equals stays null-tolerant. Equals(string, StringComparison) added. - AudioListener.volume scales every voice already playing. - Delegates made where the closed type is known carry it, so Combine of Action<int> and Action<string> (or a variant Action<object>) throws as .NET does. - InputManager invert is honoured for key and mouse axes. - ContactFilter2D.ClearDepth/ClearNormalAngle keep the bounds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ster runs pr/6010.json set 0.175x from a single run; master's own runs 38346386877 and 38226452649 on that model measure 0.267x, as does this PR's run 38519282184. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf20fd1ab8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| CilType n = names.norm(type); | ||
| mv.visitLdcInsn(org.objectweb.asm.Type.getObjectType(classToken(n))); | ||
| mv.visitLdcInsn(String.valueOf(n.typeName() == null ? n.toString() : n.typeName()).replace('/', '+')); |
There was a problem hiding this comment.
For a GENERICINST, CilType.typeName() returns only the generic definition name, so this emits the same class/name pair for every construction and Type.$of returns one cached object. Consequently, expressions such as typeof(List<int>) == typeof(List<string>) incorrectly evaluate true, and any supported dispatch or dictionary keyed by a closed generic Type conflates unrelated types. Include the constructed arguments in the emitted identity.
Useful? React with 👍 / 👎.
| private static void sameType(Delegate a, Delegate b) { | ||
| Object x = a.$typeKey; | ||
| Object y = b.$typeKey; | ||
| if (!a.$sameType(b) || x != y && x != null && y != null && !x.equals(y)) { // NOPMD CompareObjectsWithEquals |
There was a problem hiding this comment.
When a delegate is created inside generic code, delegateTypeKey() deliberately leaves $typeKey null, and this condition rejects a mismatch only when both keys are non-null. Thus delegates returned by G<int> and G<string> can be passed to Delegate.Combine(Delegate, Delegate) and accepted as the same erased Action_1 type, even though .NET must throw ArgumentException; invoking the result can then fail with an unrelated argument cast. Fresh evidence beyond the earlier closed-delegate issue is the current null-key path for open generic creation, which still bypasses the new key comparison.
Useful? React with 👍 / 👎.
| m_LookAt = v.m_LookAt; | ||
| m_Follow = v.m_Follow; |
There was a problem hiding this comment.
When Instantiate copies a GameObject hierarchy whose virtual camera follows or looks at a Transform within that hierarchy, these direct assignments retain the source Transform. copyTree has already registered the copied components, and the $copyFrom contract requires internal Unity references to pass through UnityRuntime.$remap; otherwise the cloned camera tracks the original object instead of its cloned sibling. Remap both fields, which will still preserve references to targets outside the copied hierarchy.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| private static void copyIfChanged(File from, File to) throws IOException { | ||
| if (to.isFile() && to.length() == from.length() && to.lastModified() >= from.lastModified()) { |
There was a problem hiding this comment.
When a staged class or resource changes to different bytes of the same size while the installed output has an equal or newer timestamp—for example, two builds within one filesystem timestamp tick—this returns without copying it. The state digest can correctly trigger translation or scene compilation, but installation then leaves the previous class or image in classesOut, causing the packaged application to use stale output. Compare the file contents, or always replace an output after its producing step reruns, rather than treating this metadata as identity.
Useful? React with 👍 / 👎.
| // A copy, because a listener may add or remove one. | ||
| Object[] now = listeners.toArray(); | ||
| for (int i = 0; i < now.length; i++) { // NOPMD ForLoopCanBeForeach | ||
| ((UnityAction) now[i]).Invoke(); |
There was a problem hiding this comment.
When a listener added with AddListener targets a Unity object that is later destroyed while the event owner survives, this unconditionally invokes the retained delegate, so managed code on the destroyed component continues to run. Unity's InvokableCall checks AllowInvoke and suppresses delegates whose UnityEngine.Object target has been destroyed, matching the check already applied to persistent listeners above (Unity reference source); apply the same target check to runtime listeners, including multicast invocation lists.
Useful? React with 👍 / 👎.
| public void InvokeRepeating(String methodName, float time, float repeatRate) { | ||
| UnityRuntime.invokeLater(this, methodName, time, repeatRate > 0f ? repeatRate : 0f); |
There was a problem hiding this comment.
When repeatRate is zero, negative, or a positive value no greater than 0.00001f, this either converts the request into a one-shot invocation or accepts a rate Unity rejects, so a configuration error silently calls the method with different timing. Unity's MonoBehaviour.InvokeRepeating throws when the rate is not greater than 0.00001f (Unity reference source); validate the rate instead of passing zero to the ordinary invocation scheduler.
Useful? React with 👍 / 👎.
…ode, event and clone fixes - typeof of a constructed generic type carries its arguments, so typeof(List<int>) != typeof(List<string>); one cached Type per closed type. - Delegates made in generic code that has a per-closed-type context get a closed type key; the remaining null-key cases are documented. - UnityEvent skips runtime listeners whose target is destroyed and ignores a duplicate AddListener, as documented. - CinemachineVirtualCamera remaps Follow/LookAt when a hierarchy is cloned. - UnityProjectBuilder replaces every output after a step reran, and writes its state after install. - InvokeRepeating comment states what the docs specify. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RAM is bimodal per round (~0.118x or ~0.04x); run 38536963726 had 3 of 5 rounds low and failed the gate as an improvement. Pooled with the PR's earlier run and three master runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 821ae3e82d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pitch = value; | ||
| if (playing && output != null) { | ||
| output.pitch(voice, value); |
There was a problem hiding this comment.
When a script assigns a value outside [-3, 3], such as source.pitch = 10, this stores and forwards the value unchanged, so get_pitch(), playback position, and isPlaying advance at 10× speed. Unity clamps AudioClip pitch to [-3, 3], so clamp the value before updating the playback clock or output voice. See Unity's AudioSource.pitch contract.
Useful? React with 👍 / 👎.
| public static String Concat(Object[] parts) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (int i = 0; i < parts.length; i++) { // NOPMD ForLoopCanBeForeach |
There was a problem hiding this comment.
When translated code calls string.Concat((object[])null), dereferencing parts.length throws a Java NullPointerException, which Interop.wrap exposes as NullReferenceException. .NET requires ArgumentNullException for a null array, so catch clauses observe the wrong exception; add an explicit null check here and in the sibling String[] overload. See the .NET String.Concat(Object[]) contract.
Useful? React with 👍 / 👎.
| float r = raw(axes[i]); | ||
| if (r != 0f) { | ||
| v = r; | ||
| } |
There was a problem hiding this comment.
When InputManager.asset defines multiple axes with the same name, this returns the last nonzero raw value rather than the value with the largest absolute magnitude. For example, if an earlier keyboard definition reports 1 and a later mouse definition reports 0.2, GetAxisRaw incorrectly returns 0.2, while GetAxis immediately below applies the correct selection rule. Compare magnitudes here as well; Unity specifies that same-named axes are queried together and the largest absolute value is returned in the Input Manager manual.
Useful? React with 👍 / 👎.
| public void Play() { | ||
| begun = true; |
There was a problem hiding this comment.
When a parent particle system calls Play() without arguments, this implementation starts only that system, although Unity's parameterless form uses withChildren = true; child particle systems therefore remain stopped in common effects composed from a root and several child emitters. The parameterless Stop, Pause, and Clear methods have the same mismatch. Route these default overloads through child propagation, as specified by Unity's ParticleSystem.Play contract.
Useful? React with 👍 / 👎.
| public void CompressBounds() { | ||
| } |
There was a problem hiding this comment.
When tiles at an edge are removed, or when grow() has added its eight-cell spare padding, calling CompressBounds() leaves origin and size unchanged because this method is a no-op. Scripts that use those properties to iterate the occupied area therefore continue traversing empty cells and observe bounds different from Unity; shrink the storage to the remaining occupied cells, as required by Unity's Tilemap.CompressBounds contract.
Useful? React with 👍 / 👎.
| @Override | ||
| public void Reset() { | ||
| throw new NotSupportedException(); | ||
| } |
There was a problem hiding this comment.
When translated code obtains a List<T> enumerator through IEnumerator and calls Reset(), this always throws NotSupportedException; the .NET list enumerator instead verifies that the list was not modified and resets its index and current value, allowing enumeration to start over. Implement that behavior so valid callers do not fail solely after translation; see the .NET reference implementation.
Useful? React with 👍 / 👎.
…w axis, null params, pitch clamp - List and Dictionary enumerators reset (InvalidOperationException once the collection changed), as .NET does. - Tilemap bounds are kept apart from the storage and CompressBounds shrinks them to the occupied cells. - Parameterless ParticleSystem Play/Stop/Pause/Clear include children. - GetAxisRaw over same-named axes answers the largest magnitude. - string.Concat/Format with a null array or format throw ArgumentNullException. - AudioSource.pitch is clamped to [-3, 3]. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0890827888
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| float speed = pitch < 0f ? -pitch : pitch; | ||
| if (clip == null || !((paused ? pausedAt : now()) - startedAt // NOPMD LogicInversion | ||
| < (clip.get_length() - startedFrom) / (speed > 0f ? speed : 1f))) { |
There was a problem hiding this comment.
When pitch is zero, position() remains stationary but this fallback divisor of 1f still expires a non-looping voice after the ordinary remaining duration; with a negative pitch, expiration is likewise measured toward the clip's end even though playback is moving toward time zero. For example, starting a ten-second clip at time = 2 and pitch = -1 reports isPlaying for eight seconds instead of two, while pitch = 0 eventually reports false despite paused playback. Base expiration on pitch direction and never advance it for zero pitch; Unity explicitly describes negative and zero pitch as reverse and paused playback in the AudioSource pitch contract.
Useful? React with 👍 / 👎.
| if (!touchesArrive) { | ||
| routeFormTouch(e.getX(), e.getY(), true, true, false); |
There was a problem hiding this comment.
In an embedded GameView, this fallback listener receives every press on the top-level form, including presses on sibling controls. Until touchesArrive becomes true—and permanently on ports where the direct multi-touch callback is why this fallback is needed—clicking a toolbar or other host UI therefore marks the game's pointer down and can trigger mouse-driven gameplay. Only start a fallback gesture when the press lies within the view bounds, while still allowing an outside release to finish a gesture that began inside.
Useful? React with 👍 / 👎.
| if (best != null && canvas.sortingOrder < bestOrder) { | ||
| continue; |
There was a problem hiding this comment.
When overlapping canvases differ in render mode or sorting layer, this chooses the hit canvas using only sortingOrder. Rendering in DrawList.inOrder instead places overlay canvases after camera canvases and orders camera content by group, sorting layer, sorting order, and depth, so a camera-space canvas with a larger numeric order can receive a click through an overlay canvas that is visibly on top; camera canvases on different sorting layers can be inverted similarly. Rank raycast candidates with the same ordering keys used for drawing.
Useful? React with 👍 / 👎.
| output = o; | ||
| if (o != null) { | ||
| o.listenerVolume(listenerVolume); | ||
| } | ||
| // An output that arrives in the middle of a pause -- a view shown |
There was a problem hiding this comment.
When a UnityGameView leaves a form, deinitialize() stops every device voice and clears the output without calling AudioSource.Stop, so each source retains its logical playing state and playback position. When the same view is shown again, installServices() supplies a fresh output, but this setter only applies listener volume and pause state and never recreates or seeks those retained voices. Consequently, background music reports isPlaying == true after navigation away and back while remaining silent indefinitely unless the script explicitly calls Play again; reinstall active source voices at their current positions or preserve them across the detach.
Useful? React with 👍 / 👎.
…rection; GameView host presses - AudioSource re-creates every playing source's voice at its position when a different output is installed (a view shown again). - UI raycast ranks canvases with the same keys as drawing. - A paused (pitch 0) voice never expires; a reversed one ends at 0. - GameView's form-level fallback starts a gesture only for a press inside the view; a drag or release outside still ends one begun inside. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 184be1d936
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public static boolean Any(IEnumerable_1 source, Func_2 predicate, Class t) { | ||
| IEnumerator_1 e = walk(source); |
There was a problem hiding this comment.
When predicate is null, this overload returns false for an empty source and throws a translated NullReferenceException only after encountering an element, rather than consistently throwing ArgumentNullException before enumeration. This affects the other predicate/selector overloads in this class as well and can send translated exception handling down the wrong path; validate each delegate before calling walk, as required by the .NET Enumerable.Any contract.
Useful? React with 👍 / 👎.
| public static Object ElementAt(IEnumerable_1 source, int index, Class t) { | ||
| IEnumerator_1 e = walk(source); | ||
| try { |
There was a problem hiding this comment.
When index is negative, at == index can never succeed, so this method consumes the entire source before throwing; on an infinite iterator it never returns, and on a stateful iterator it runs side effects that .NET never performs. Validate the index before obtaining or advancing the enumerator, since the ElementAt(IEnumerable<T>, int) contract requires an immediate ArgumentOutOfRangeException for a negative index.
Useful? React with 👍 / 👎.
| if (v > 1e15) | ||
| { | ||
| return negative ? "-Infinity" : "Infinity"; | ||
| } |
There was a problem hiding this comment.
For any finite component whose magnitude exceeds 1e15, this branch emits Infinity, so values such as new Vector3(1e20f, 0, 0).ToString() become indistinguishable from actual infinity and can corrupt logs or string-based serialization. Unity formats these components with invariant F2 (and colors with F3), which preserves finite values; use a formatting path that handles the full float range rather than treating a long-conversion limit as infinity, matching the Unity Vector3.ToString implementation.
Useful? React with 👍 / 👎.
| public static void Clear(Object array, int index, int count) { | ||
| if (array instanceof MdArray) { |
There was a problem hiding this comment.
When translated code calls Array.Clear(null, index, count), vectorLength throws NullReferenceException, so a caller catching ArgumentNullException observes the wrong path. Add an explicit null check before dispatching to multidimensional or vector storage (including the struct-array helper), as the .NET Array.Clear contract requires ArgumentNullException for a null array.
Useful? React with 👍 / 👎.
| // Unity runs a coroutine up to its first yield before StartCoroutine | ||
| // returns. | ||
| boolean ended = advance(c); |
There was a problem hiding this comment.
When another object, event, or retained reference calls StartCoroutine on a component whose GameObject is already inactive in the hierarchy, this unconditionally advances the iterator to its first yield, allowing arbitrary coroutine side effects before runCoroutines notices the inactive owner and stops it. Unity does not start coroutines on inactive GameObjects, consistent with its documented rule that deactivating a GameObject stops its attached coroutines; check owner.gameObject.activeInHierarchy before calling advance so the routine never executes in this case (Unity StartCoroutine documentation).
Useful? React with 👍 / 👎.
… text - StartCoroutine on an object inactive in the hierarchy runs nothing and answers null. - Array.Clear, Enumerable delegate overloads, Concat and ElementAt check their arguments before touching the source, as .NET does. - Value types format any finite component in full (F2/F3 invariant); only a real infinity prints "Infinity". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6598efee7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protected void deinitialize() { | ||
| if (holdsMultiKey) { | ||
| holdsMultiKey = false; | ||
| releaseMultiKey(); | ||
| } |
There was a problem hiding this comment.
When this view is removed while a physical key is held, its later release is delivered to the new focus owner rather than this component, but deinitialize() leaves both the held map and the queued/runtime key state intact. Showing the same view again therefore makes Input.GetKey remain true, and the next press of that key is ignored because it is already present in held; clear the local state and enqueue or otherwise apply releases for every held Unity key during detachment.
Useful? React with 👍 / 👎.
| if (audio != null) { | ||
| audio.stopAll(); | ||
| AudioSource.$output(null); | ||
| audio = null; |
There was a problem hiding this comment.
When two UnityGameView instances are shown, the most recently initialized one installs its UnityGameAudio as the process-wide AudioSource output, but detaching either view unconditionally replaces that output with null. Detaching the older view therefore disconnects the still-visible newer view, so subsequent AudioSource.Play calls are silent; clear the output only if this view still owns it, or manage audio ownership with the same multi-view accounting used above.
Useful? React with 👍 / 👎.
| mv.visitMethodInsn(INVOKEVIRTUAL, c, "$copy", "()L" + c + ";", false); | ||
| mv.visitFieldInsn(PUTFIELD, name, fieldName, descriptor); |
There was a problem hiding this comment.
When a serialized script field is a struct that itself contains a UnityEngine.Object reference, such as a Transform pointing to a sibling in the prefab, this path invokes the ordinary value $copy() and never runs that nested reference through UnityRuntime.$remap. Instantiating the hierarchy therefore leaves the clone's struct pointing into the source hierarchy instead of the cloned sibling; emit a serialization-aware struct copy here, and use it for struct-array elements, that remaps nested Unity references.
Useful? React with 👍 / 👎.
| UnityRuntime.runFrameCalls(); | ||
| return; |
There was a problem hiding this comment.
When a key is pressed and released while pauseWanted is true, this branch returns without consuming pending; the first resumed frame then replays both events, making Unity scripts observe GetKeyDown and GetKeyUp for input that happened entirely while paused. Drain paused key events here, ignoring new presses while retaining only releases needed to clear keys that were already held before the pause, analogous to the mouse handling.
Useful? React with 👍 / 👎.
| public static boolean op_Implicit(RaycastHit2D hit) { | ||
| return hit.collider != null; | ||
| } |
There was a problem hiding this comment.
When a script retains a RaycastHit2D and its collider is subsequently destroyed, the Java reference remains non-null, so this operator continues returning true and if (hit) can traverse the destroyed collider. Unity's custom object equality makes a destroyed object compare equal to null, while a raycast hit is valid only when its collider is valid; use Object.op_Implicit(hit.collider) rather than Java reference nullness. See Unity's Object equality contract and RaycastHit2D boolean contract.
Useful? React with 👍 / 👎.
| @Override | ||
| public void delete(String key) { | ||
| Preferences.delete(PREFIX + key); | ||
| } |
There was a problem hiding this comment.
When a game repeatedly calls DeleteKey and later sets the same key again, this removes only the value, so set() sees it as absent and appends another copy of its name to INDEX. The persistent index therefore grows without bound and makes DeleteAll progressively slower, potentially reaching platform preference-size limits in reset-heavy applications; remove the key from the index when deleting it or avoid appending names already recorded there.
Useful? React with 👍 / 👎.
…cast hits, nested struct remap, prefs index - Key presses made while paused or with the clock held are dropped and only releases pass on; a view taken off its form releases every held key. - Detaching one of two views clears the audio output only if it is still that view's. - RaycastHit2D is false once its collider is destroyed. - Instantiate remaps Unity references inside serialized structs (fields, arrays, lists) instead of a plain value copy. - PlayerPrefs.DeleteKey removes the name from the persisted index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Summary
A Unity 2D project (C# scripts, scenes, prefabs, sprites, project settings) dropped into
common/src/main/unity, or imported withcn1:import-unity-project, builds unmodified as a Codename One application for every target. No Unity player and no .NET runtime ship with the app.maven/cil-translator: reads the assembly the C# compiler produced and writes one class file per C# type; compiles scenes, prefabs, animator controllers, clips, tile maps and particle systems to reflection-free Java (UnityAppImpl).maven/unity-compat: the runtime. A .NET base library in Java, theUnityEngineAPI overcom.codename1.gaming(2D rendering, Box2D physics with exact casts and overlap queries, Animator, tile maps, particles, uGUI, touch, audio,PlayerPrefs,Resources), plus our own implementations of the documented TextMesh Pro and Cinemachine API.UnityProjectBuilderinbuild-engine, thecompile-unityandimport-unity-projectMaven goals, the GradlecompileUnitytask, the archetype profile, and release publishing behind-Dunity-compat(the modules need the .NET SDK; the framework build does not).UnityApplication(createForm,onProjectInstalled,onStarted,callInFrame), so a game sits inside a Codename One form and uses the rest of the framework.Unity-Interop.asciidoc.Clean room: no Unity source, assemblies or package source were read or shipped; the stub
UnityEngine.dllis compiled from our own sources.Changes outside the new modules
Bugs this work uncovered, fixed at the root, each with a regression test:
POP2/DUP2/DUP_X2),MULTIANEWARRAYdimension pops, a call result compared viaFCMP/DCMPevaluated more than once,SWAPlosing the lower value.arraycopy/clone of an array of primitive arrays took the primitive path.GameView: a key repeat released a held key after 800 ms; a focused hosted child kept the multi-touch arrays from the view.World.popContact; the broad phase allocated a merge buffer every step above 31 pairs (now an in-place sort, checked againstArrays.sortover randomized pair sets).Behavior changes that affect every application:
-ffp-contract=off.floatresult withMath.fround.Both make float arithmetic match the JVM bit for bit. Their performance cost on ordinary applications has not been measured.
Verification
scripts/unity-compat-samples) produce byte-identical traces on the JVM, ParparVM native and JavaScript (maven/integration-tests/unity-compat-test.sh --parparvm).cil-translatorandunity-compat: SpotBugs 0, Checkstyle 0, PMD forbidden list clean, cast-semantics clean.MapViewMarkerLabelTesttimeout on a loaded machine; passes alone).Not verified
.github/workflows/unity-compat.ymland therelease.ymlchanges have never run on CI.GameInputstill reads edge flags that a press landing mid-frame can miss on ports that render off the EDT; left for a separate decision.🤖 Generated with Claude Code