-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements to FreeJ2ME's game compatibility by adding some missing classes (redone) #145
base: master
Are you sure you want to change the base?
Improvements to FreeJ2ME's game compatibility by adding some missing classes (redone) #145
Conversation
Games like 3D Pinball Timeshock use the -1 index as a record ID for some reason, so in cases where the index is lower than 0, the code truncates the index to 0. Allowing Timeshock to boot, but failing a bit later due to the current 3D implementation.
Before, when the screen was rotated on the libretro frontend, the pointer coordinates in the java file were reversed correctly, but some extra calculations to account for the 90 degrees counterclockwise rotation were not done, making the pointer be "reversed" in the y axis, so the lower the pointer was in the screen, the higher the y value it had in the rotated mode. Now it works as intended.
Some games rely on those classes to be present, like my 320x240 copy of Asphalt Nitro. The 240x320 version of the same game doesn't seem to use them however. All files are based off of Java's ME JSR Community Process docs.
Those files allow games like 3D Formula Extreeme! to boot. Code based off of Java's ME JSR Community Process docs as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the javax.microedition.io.file
classes yet, so I'd double-check those against the docs you have.
It doesn't seem like that package has an unusual history. Here's what I found for it with a quick search. It may be worth comparing against what you have:
https://blackberry.com/developers/docs/5.0.0api/javax/microedition/io/file/package-summary.html
src/javax/bluetooth/DeviceClass.java
Outdated
*/ | ||
package javax.bluetooth; | ||
|
||
public abstract class DeviceClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this declared abstract?
*/ | ||
package javax.bluetooth; | ||
|
||
public abstract class DiscoveryAgent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this declared abstract?
javax.microedition.lcdui.Canvas
is an example of a class that needs to be abstract. You'll see that the javadocs clearly label it as such:
https://docs.oracle.com/javame/config/cldc/ref-impl/midp2.0/jsr118/javax/microedition/lcdui/Canvas.html
*/ | ||
package javax.bluetooth; | ||
|
||
public abstract interface DiscoveryListener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this declared abstract?
src/javax/bluetooth/LocalDevice.java
Outdated
import javax.microedition.io.Connection; | ||
|
||
|
||
public abstract class LocalDevice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this declared abstract?
src/javax/bluetooth/UUID.java
Outdated
|
||
public abstract String toString(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it doesn't really matter in Java, there's no reason not to add a newline at the end.
src/javax/bluetooth/UUID.java
Outdated
*/ | ||
package javax.bluetooth; | ||
|
||
public abstract class UUID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this declared abstract?
https://docs.oracle.com/javame/config/cldc/opt-pkgs/api/bluetooth/jsr082/javax/bluetooth/UUID.html
public void truncate(long byteOffset); | ||
|
||
public long usedSize(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it doesn't really matter in Java, there's no reason not to add a newline at the end.
@@ -211,7 +211,7 @@ public void run() | |||
} | |||
else | |||
{ | |||
Mobile.getPlatform().pointerDragged(mousey, mousex); | |||
Mobile.getPlatform().pointerDragged(-mousey+lcdWidth, mousex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't important, but lcdWidth-mousey
makes the intent more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that was because i wrote the properties of doing a 90 deg counterclockwise rotation, then corrected the pointer position it by adding the width, perhaps the translation to code was a bit too direct. But yes, that recommendation makes it much easier to read. I'll be updating it later.
I've based my review on JSR 82: |
Oh shoot, i forgot to mark this one as a draft. I'm really Sorry! The reason they are abstract for now is because i just stubbed them as much as i could to test if they were already causing any effect. i¨ll be converting it to a draft shortly. |
The implementations were mostly in line with the javadocs as of the previous commit. This one makes them nearly identical to the docs.
|
||
public DataOutputStream openDataOutputStream(); | ||
@Override /* Throws IOException in the docs, but not on the overriden class */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" Throws IOException in the docs, but not on the overriden class" What does this mean? What other class? Remember that this is an API. That means it is entirely separate from any specific application that uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means that this DataOutputStream method is described as throwing an IOException in the javadocs. However, our current implementation (the compiler at least complained about not overriding a function with the same name as that) doesn't. I'll clean it up later when i do a second pass on all those files.
@@ -18,11 +18,15 @@ | |||
|
|||
import java.util.Enumeration; | |||
|
|||
public abstract class FileSystemRegistry | |||
public class FileSystemRegistry extends Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extends Object
is unnecessary as all Java classes extend Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that was just to make an exact transcription of the docs, i still need to clean some things up.
This folder is a prerequisite for some of the incoming implementations of the recent bluetooth files, which even in their mostly stubbed state still needs these obex files to be present.
The code in them should now be mostly identical to the javadocs.
Hmm... after looking at all the files again, seems this PR is ready for review... at least to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really give it a complete once-over. There's enough to keep you busy anyway. Also we try to use tabs for indenting, spaces for alignment. Your editor should be able to do that conversion automatically.
src/javax/bluetooth/UUID.java
Outdated
|
||
public boolean equals(Object value) { return false; } | ||
|
||
public int hashCode() { return uuid.hashCode(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line has a lot of problems. First, uuid
is null and will never be set because, even though it's public by default, no game exists that would try to set this property as it's not part of the standard. Even if it was initialized, calling this method will cause a stack overflow. Java does not have tail call optimization, but even if it did this would still be an infinite loop.
If you're just stubbing this out:
The rules for this, believe it or not, will let you get away with returning a the same integer for every value.
From: https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#hashCode()
"It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results. However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hash tables."
However "If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result. " This is probably why this overrides Object.hashCode
src/javax/bluetooth/UUID.java
Outdated
public class UUID | ||
{ | ||
|
||
UUID uuid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be declared private and it shouldn't be a UUID -- that's the thing you're defining here, after all. The way you try to use this later also causes a few problems.
I'd recommend using 4 longs, using each as a 32-bit unsigned integer to store the UUID. It'll make things a lot simpler.
src/javax/bluetooth/UUID.java
Outdated
|
||
public int hashCode() { return uuid.hashCode(); } | ||
|
||
public String toString() { return BASE_UUID_VALUE; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return whatever it is you're using to store the uuid converted to a string (hexadecimal) this could be a 16, 32 or 128 bit value, but this should always return the 128 bit version. An odd quirk, leading zeros should not be included, meaning a 128-bit uuid will not necessarily turn in to a 32 character long string.
I know you're just stubbing this out, but it's not a very complicated class to just implement.
src/javax/bluetooth/UUID.java
Outdated
public UUID(String uuidValue, boolean shortUUID) throws IllegalArgumentException, NullPointerException, | ||
NumberFormatException { System.out.println("UUID Value:" + uuidValue); } | ||
|
||
public boolean equals(Object value) { return false; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just check strings returned from toString using the String.equals
method. Be aware that you'll need to cast value
to a UUID
first.
src/javax/bluetooth/UUID.java
Outdated
{ | ||
|
||
UUID uuid; | ||
private static final String BASE_UUID_VALUE = "0x0000000000001000800000805F9B34FB"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very wrong. First, the "0x" prefix shouldn't be included in a string representing a UUID. Second, storing this as a string isn't terribly useful. It would be better spread this out over 4 longs. Remember that the base is used to "promote" 16 and 32-bit UUIDs to 128 bits. (The toString
and equals
methods require this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well, i'll be applying those changes, including the others above.
public class IllegalModeException extends RuntimeException | ||
{ | ||
|
||
public IllegalModeException() { } /* Constructs an instance of the class with its stack trace */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this comment or its twin are helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those were just copied from the docs, but they're a bit vague, yes. Makes more sense to drop them altogether.
@@ -548,6 +548,7 @@ public int nextRecordId() throws InvalidRecordIDException | |||
//System.out.println("> Next Record ID (idx:"+index+" cnt:"+count+")"); | |||
if(keepupdated) { rebuild(); } | |||
if(index>=count) { throw(new InvalidRecordIDException()); } | |||
if(index < 0) return elements[0]; // If an invalid index is received, truncate it to 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what 'truncate' means. Also, how would index
ever become less than 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i agree that using "truncate" there doesn't make sense.
And as for the index becoming less than 0, a game i tested some days ago (Pinball 3D Timeshock) does that:
It's the game that i fixed by adding that index check. I have no idea why it tries to access a recordId on index -1, but forcing it to move to index 0 allowed it to save its data afterwards:
Previously it would only save the "highScore" data, not the "CameraType".
Still, that might not be the best solution to cases like those, i'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. I'd like to figure out what happened. There shouldn't be any way to get index below zero. We should look for any places where index is reduced and put the check there (or force it to zero).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to un-comment every single System.out line in the RecordStore class to see what was going on while it tried to save and load the rms files, and it turns out that the game doesn't even decrement the index while reading, it just goes for a negative one an an invalid ID right out of the gate, at least it's what the logs tell me (either that, or i'm interpreting the messages incorrectly):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are a few places in enumeration
where it could potentially go below zero. previousRecord
and rebuild
both assume count is greater than zero. I'm surprised this escaped notice until now. Count should also be initialized in the constructor... ugh... I'll see about updating this later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do those if you want, should help keeping things cleaner as they'll all be in the same PR.
Edit: Hmm... i think i fixed it. Just initialized count as 0 in the enumeration constructor and added a check for when index < 0 in the rebuild() method. I commented that check i placed inside nextRecordId() and so far it still works fine in Pinball 3D Timeshock:
I decided to test DOOM II RPG as well since it uses rms a bit more extensively compared to other games like asphalt 6 (still works fine) and so far it's still ok:
Sure, those other games shouldn't be affected by those changes, but it's always good to do a little testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about the number of pull requests. I'd rather see a bunch of smaller ones than a big omnibus. If something needs reverted, it's helpful if the PRs are more focused. Ideally, a PR will address a specific issue. It's not uncommon for more experienced contributors to create an issue right before they submit a PR that addresses it.
It also makes the review process simpler. It takes a lot of time to go over these, and I know I'm not spending nearly enough time on most of the files just because of how many there are. This one alone has something like 22 files changed. I'd bet that most of these files would have already been approved had they been standalone or in smaller groups.
Remember that most of this project is tending toward an ideal. Just like an emulator has the hardware as a standard, this project has the standards as a standard. The goal isn't to get a game to work, but to get closer to that ideal. What matters is correctness, above all. It wasn't uncommon early on to have correct changes break games.
Don't try to make the game work. That's the easy thing to do, but it's usually wrong. It's better to find out what problems with our implementation are keeping the game from working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i'm not worried about making things work with that change above. I just tried to do a few patches to the RecordStore file based on your pointers and it seems to have worked correctly.
And i'll definitely keep that smaller PR advice in mind.
src/javax/obex/Authenticator.java
Outdated
{ | ||
|
||
PasswordAuthentication onAuthenticationChallenge(String description, | ||
boolean isUserIdRequired, boolean isFullAccess); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why split this across two lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to make the lines shorter, kinda like it's done in older C standards. Maybe it isn't useful here, so i'll get those back to one longer line.
The UUID implementation might still be incorrect, i'm basing it almost entirely on the data from the docs.
Just gave it a shot on implementing the UUID (alongside many adjustments to the other files added in this PR). If there's anything else to be done, or the implementation is incorrect, please share. I'm not used to doing those manipulations used in the UUID. |
Instead of checking and working around cases where the rms index would be lower than zero, it's better to simply find a way to make sure it won't ever go below zero at all. This reverts commit 3ee6a3c.
src/javax/bluetooth/UUID.java
Outdated
@Override | ||
public String toString() | ||
{ | ||
String uuidString = Long.toHexString(UUID1) + Long.toHexString(UUID2) + Long.toHexString(UUID3) + Long.toHexString(UUID4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't do what you want.
Consider the following values for UUID1-4: 0x00000000
, 0x0011223344
, 0x00667788
, 0x0000BBCC
The expected output from toString
is: "11223344006677880000BBCC"
Your function will produce: "11223344667788BBCC"
as Long.toHexString
will not prepend leading zeros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, it figures I wouldn't catch a mistake until I posted this. Ignore the extra 8 bits at the front of UUID2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I think i can just format those long values to a 8 char hex string through String.format, not needing to over-complicate stuff by getting that toHexString result and manipulating it.
src/javax/bluetooth/UUID.java
Outdated
public String toString() | ||
{ | ||
String uuidString = Long.toHexString(UUID1) + Long.toHexString(UUID2) + Long.toHexString(UUID3) + Long.toHexString(UUID4); | ||
return uuidString.toUpperCase().replaceFirst("^0+(?!$)", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the bluecove docs (which seem more complete than the JSR 82 docs from Oracle) Only leading zeros should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think leaving that more robust regex in would cause problems given the expected UUID format, but i'll change it to remove only the zeros nonetheless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little more complicated than I expected. I wrote this up last night, along with a few tests and it seems to be working correctly. It's pretty heavily commented, which should be helpful as while the conversions to and from strings are very simple, they can be a bit tricky if you're not used to bit twiddling.
I'll give myself a B- on the quality scale. There's room for improvement here, but 'where' is not super obvious to me at the moment and I really should be doing other things right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mine currently works as well after i dropped those toHexString methods and adjusted the value space of the baseUUID variables. But things such as making the UUID an array are good ideas, i'll be incorporating some of those changes into my code.
@recompileorg had some suggestions in the form of code, and a few of the implementations could be ported over to this current one.
No idea how that empty first line escaped my sight for so long... Should be formatted correctly now.
Cleaned this PR up a bit. Now it only has those new files. The pointer, rms negative index and a few other minor fixes were moved into their own pull requests. |
src/javax/bluetooth/UUID.java
Outdated
|
||
if (shortUUID) | ||
{ | ||
String formattedUuidValue = String.format("%08X", uuidValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's crashing on UUID t2 = new UUID("1234", true);
>java driver
test...
expected: 4660
hashcode: 4660
expected: 123400001000800000805F9B34FB
toString: 123400001000800000805F9B34FB
Exception in thread "main" java.util.IllegalFormatConversionException: x != java.lang.String
at java.base/java.util.Formatter$FormatSpecifier.failConversion(Formatter.java:4426)
at java.base/java.util.Formatter$FormatSpecifier.printInteger(Formatter.java:2938)
at java.base/java.util.Formatter$FormatSpecifier.print(Formatter.java:2892)
at java.base/java.util.Formatter.format(Formatter.java:2673)
at java.base/java.util.Formatter.format(Formatter.java:2609)
at java.base/java.lang.String.format(String.java:3292)
at UUID.<init>(UUID.java:49)
at test.<init>(test.java:13)
at driver.<init>(driver.java:10)
at driver.main(driver.java:5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... i was confusing that uuidValue with the one in the constructor above, treating it as a long when i should be treating as a string instead. Changes incoming.
src/javax/bluetooth/UUID.java
Outdated
{ | ||
int length = uuidValue.length(); | ||
|
||
if(uuidValue == null || length==0 || length>32 || (shortUUID && length>8) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linLe 41 should throw a NullPopinterException
if uuidValue
is null. There isn't a reason to check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it's a bit redundant after eyeing the code as a whole.
Copying the PR message from the previous similarly-named one: More general improvements to the game compatibility, this time by adding some classes not available to them prior and working around a few weird behaviors that some games relied on, enabling many to go beyond the boot sequence and even reach near-perfect gameplay now.
The screen pointer on the libretro frontend was also fixed, since it wasn't pointing at the correct coordinates when the screen was rotated, which many Touchscreen games require.- Moving this one into a separate commit, just like the "negative index" fix.But this time, those new implementations and stubs are clean and based off of Java's ME JSR Community Process docs, since many can't be found on the main javadocs pages.
The docs: https://jcp.org/en/jsr/platform?listBy=1&listByType=platform
Edit: This one fixes #148, although the issue was created after this PR.