Zay-ES-net, latest commit to fix Reverted HostedEntityData.getStringInfo() to send -1 instead of null

asser_fahrenholz · April 28, 2019, 8:01pm

This line here:

jMonkeyEngine-Contributions/zay-es/blob/741b3e16a5e53d7977ffe5b19707f03982d3b011/extensions/Zay-ES-Net/src/main/java/com/simsilica/es/client/RemoteStringIndex.java#L99


        // Check the result again because between unlock
        // and lock something else might have requested the same
        // string.
        Integer result = idIndex.get(s);
        if( result != null ) {
            return result;
        }
        
        // Else ask remote
        result = remote.getStringId(s);
        if( result > 0 ) {
            idIndex.put(s, result);
            stringIndex.put(result, s);
            return result;
        }
        return -1;
    } finally {
        indexLock.writeLock().unlock();
    }               
}

Should include 0. The very first index that is generated, is 0, not 1.

At least this is bugging me on my end:

Server-side:

and I’m getting -1 when asking remoteentitydata for legacy. Am I doing something wrong?

Kind regards,
AssoFohdz

pspeed · April 28, 2019, 8:14pm

And we continue to learn valuable lessons about pushing PRs without being able to test them myself… sigh…

In this case, I don’t even know why I’m not hitting the issue locally.

Without testing/running/etc., I’m going to guess that the check should be >= 0.

I must locally use up some string IDs and then never use them on the client or something. That’s the only way I can explain not seeing this.

asser_fahrenholz · April 28, 2019, 8:32pm

I’m not sure if that’s the only place affected by the >0 check though. This is just where I ran into it

pspeed · April 28, 2019, 8:38pm

It should be the only place as it’s the case trying to decide whether or not to cache the value passed back from the server.

Just based on the logic alone, ‘0’ is clearly a gap here. The check should be >= 0 otherwise 0 gets converted to -1. I missed it on code review and somehow didn’t hit it in testing.

Edit: it’s also funny how things evolve because idiomatically speaking, I would not normally have code the block this way and would have opted for one return result at the end. The upside being you wouldn’t have hit this issue. The downside being that result=0 would never have been cached and we’d never know. Makes me think.

Ali_RS · April 29, 2019, 5:31am

Ah sorry, I made a mistake again on this fix for the second time I guess , I would prefer to not be the one to fix it this time.

asser_fahrenholz · April 29, 2019, 6:01am

Not sure how to test it locally. I have the Zay-ES-Net fix in a local repo, I build the .jar-file. Now I dont know how to use this local updated jar in my gradle-project (instead the one from jcenter). Any pointers?

Ali_RS · April 29, 2019, 6:09am

In your IDE right click on your local repo of Zay-ES-Net → tasks → install, this will install it to your maven local (USER_HOME/.m2) now you can use it in your gradle project by adding it in dependencies.

Edit:
Also make sure to add mavenLocal() in repositories

repositories {
    
    //Uncomment this if you install local dependencies.
    mavenLocal()
}

Edit2:
You can also install it with gradle install or gradlew install command without using an IDE.

asser_fahrenholz · April 29, 2019, 6:48am

But the Zay-ES-Net is not a gradle project. So I cannot use gradle install

And I already have mavenlocal added as repository for dependencies. But I can’t move it ahead of the remote maven-repo, because of other dependencies.

I just dont know how to add the Zay-ES-Net.jar to the local repo - cannot google my way to the command.

pspeed · April 29, 2019, 7:13am

Yes, it definitely is.

Edit: https://github.com/jMonkeyEngine-Contributions/zay-es/blob/master/extensions/Zay-ES-Net/build.gradle

Edit 2: check out zay-es. Run gradle install. Watch as both zay-es and zay-es-net are installed into your local m2 repo.

Ali_RS · April 29, 2019, 7:16am

It is, see there is already a build.gradle in it, but it also is a maven and netbeans projects too as it also has build.xml and nbproject.
If you want your IDE detects it as gradle projetc just rename build.xml to no_build.xml and nbproject to no_nbproject and restart your IDE.

asser_fahrenholz · April 29, 2019, 7:18am

Ah. Not used to projects being multiple ‘types’. I just saw it in the IDE as regular Netbeans and jumped the gun.

Will try to install locally and get my game to use that jar instead of the jcenter one.

pspeed · April 29, 2019, 7:18am

I forgot those were still in there. They can be removed, I guess. One sec.

Edit: Ah, I remember why I left them… because the examples haven’t been converted to gradle yet and I was hoping that the whole thing was still buildable in netbeans (even if I haven’t tried it for 4+ years or whatever).

pspeed · April 29, 2019, 7:22am

If you have been normally using the real version then switching your project dependencies to the -SNAPSHOT version in the build.gradle files should be enough.

asser_fahrenholz · April 29, 2019, 7:57am

It works. No issues after using a locally fixed snapshot version.

Only the fix already mentioned here was applied.

It’s not an exhaustive test, just a use-case test I guess.

pspeed · April 29, 2019, 8:04am

If someone makes a PR then I will merge it… and if you catch me at the right time, I might even push a patch release.

pspeed · May 5, 2019, 9:29am

I have changed the > check to >=… I may push a patch release in a second.

Edit: Done.

…hopefully it really fixes the problem finally.

asser_fahrenholz · May 6, 2019, 7:06am

Cool. Thanks