Fix optional texture map lookup warnings

[belgianguy]

Treat derived normal, specular, and displacement maps as optional assets and make their lookup cache safe for concurrent loading.

Fixes #414.

[Trass3r]

Now it gets loaded twice.

[belgianguy]

@Trass3r: I edited the PR, is it better now? I've been out of it for quite a while, any recommendations are welcome. :)

[tonihele]

The problem I have with this, that it is try catch, having terrible performance. locateAsset, I checked from jME source code, doesn't do that (at least with FileLocator). They just return null, no try catch.

Granted, that is why we have the cache that it is done only once. But are we ready to accept the performance hit? What other solutions exist?

Looking at the source code of jME, we can't get access to the locators without reflection, which will make it messy and fragile. Maybe we can suppress this particular log entry if it bothers us? We use this call in couple of places but I don't think it is too bad to just suppress the log message?

[belgianguy]

Fair point, I've amended the branch to suppress these messages rather than a try/catch.

[Trass3r]

The logging should just be removed from JME itself. Or at least reduced to a debug level.
If the return is @Nullable it shouldn't spam the logs and just let the application decide if it's a problem or not.

[tonihele]

Yeah, I do agree that we should create a PR to jME to not log this. But whether that ever gets accepted or not is another thing. So the best we can do is what we have done now. It is not super pretty but I think it is the only way. Later we should of course anyway have proper logging configuration with async logging and all that to remove overhead from logging...

[tonihele]

This logic I also don't quite get? found = (assetInfo != null);, that is clear, but why

            Boolean cached = TEXTURE_MAP_CACHE.putIfAbsent(textureName, found);
            if (cached != null) {
                found = cached;
            }

Why is the added complexity there? I'm not saying it is wrong, I just don't get it. We already know whether the texture was found or not because we just checked it, then for some reason we see if it was already resolved earlier (concurrency) and use that value instead. Shouldn't the new value actually be the dominant one? Maybe somebody just added it, or removed it. Either way using simply PUT as opposed to putIfAbsent we kinda have the latest value? Of course this event is highly unlikely and if we worry about it too much, double checked locking is always a trusted friend and in many cases way better than the chaos brought by simply using synchronized collections.

Sometimes the algorithm is way heavier than the cost of just locking, so you end up loosing in the throughput in the end. But this here I am not proposing the locking, just rambling :D

So just a question why? For learning purposes

[Trass3r]

Also why not cache the Texture instead of a bool

[belgianguy]

Hi @Trass3r ,

The AssetManager already caches loaded textures. This cache is mainly for remembering missing optional maps, which the asset cache cannot represent.

[belgianguy]

@tonihele: Regarding your question:

The putIfAbsent was intended to ensure that concurrent callers use one consistent cached result, but I agree that the implementation added unnecessary complexity. I’ve replaced the whole sequence with computeIfAbsent(), which expresses the intent more directly and performs the lookup once per texture name.
Assets aren’t expected to be added or removed at runtime. If that becomes supported, this cache would need explicit invalidation—using put() alone wouldn’t reliably represent the latest state during concurrent lookups either.