[ContentManager] Non-XNB asset path normalization accuracy about fna-mghistory HOT 21 CLOSED

This is the problematic Exception: https://github.com/flibitijibibo/MonoGame/blob/monogame-sdl2/MonoGame.Framework/TitleContainer.cs#L35

The actual path location in ContentManager: https://github.com/flibitijibibo/MonoGame/blob/monogame-sdl2/MonoGame.Framework/Content/ContentManager.cs#L615

Fix these and XNA accuracy's all yours.

from fna-mghistory.

Oh, and this: https://github.com/flibitijibibo/MonoGame/blob/monogame-sdl2/MonoGame.Framework/Content/ContentManager.cs#L354

from fna-mghistory.

Got it flibixed... er, fixed... I think, without removing the exception check in OpenStream or other nastiness that would have broken API compliance.

It's not too drastic a change: just a few lines to catch the argument exception, build the path relative to the program, and use File.OpenRead's in ReadRawAsset... but I'm going to mull it over for a few days and do some testing to make sure it's the best solution and doesn't break anything with XNBs resources.

Edit: No need to catch an argument exception after further refining.

from fna-mghistory.

Hm, we might actually want to stick to TitleContainer's OpenStream, actually. That whole absolute path bug is actually an inaccuracy in itself; ContentRoot should be able to handle absolute paths without a problem. EG2 actually has an #if SDL2 def for this very issue, in fact. So, if there's a way to fix the paths and also get rid of that annoyance, that'd be awesome!

from fna-mghistory.

Er, well, that or we should have an internal OpenStream that works with absolute paths. I dunno if the actual TitleContainer.OpenStream cares about this. If OpenStream's behavior is right, then we've got an even bigger/dumber problem to fix!

from fna-mghistory.

Heh, I hit the same thing here. My XNA version set the RootDirectory using an absolute path which works perfectly fine but then in FNA it required a relative root. So I think MonoGame/FNA are in the wrong on this one; ContentManager.RootDirectory should support being an absolute path.

TitleContainer itself should only work with relative paths as the entire point of that API is to expose the files and content shipped with the game. It was made primarily for Xbox where you have StorageDevice/StorageContainer for game saves and TitleContainer for shipped files. Therefore it doesn't make sense for TitleContainer to support absolute paths.

That said, I have no idea whether or not absolute paths work with TitleContainer on Windows; can't say I've ever tried.

from fna-mghistory.

Hm, either way, I'm guessing we need to implement an internal OpenStream for ContentManager to use. The use of TitleContainer in that file is probably just wrong to begin with. This may apply to other files as well; git grep OpenStream and git grep 'File\.Open' are going to be the two searches we care about for this issue.

from fna-mghistory.

Oh wow, ContentManager basically already has one:

https://github.com/flibitijibibo/MonoGame/blob/monogame-sdl2/MonoGame.Framework/Content/ContentManager.cs#L218

So basically we just need to get this to NOT use TitleContainer, then ensure that all the other OpenStream calls in this class use the one in ContentManager, then this should be okay. I'll let you guys figure this out, since together you've both got ContentRoot and raw asset paths to fix at the same time.

from fna-mghistory.

That OpenStream in ContentManager appends ".xnb" to everything and calls TitleContainer.OpenStream, which is necessary to the way ReadAsset and ReloadAsset try to load the XNBs.

There are two problems with modifying the way the code works:

1. The code will always try to load the XNB asset first, which is good behavior as far as compatibility because if your XNA project had asset.png.XNB and asset.png in a directory, it would load the XNB too.

2. This means we still need to call TitleContainer.OpenStream from ContentManager's OpenStream and enforce XNA's same path checking. And, according to everything I've read, TitleContainer.OpenStream is correct to throw an exception for absolute paths - even on Windows.

My approach to fixing it was this:

A) Let it continue to try to loading the XNB first, which results in the ContentLoadException. Before trying to load as a non-content file, modify the asset name to be the absolute path using the game directory as a base:

            catch (Exception ex)
            {
                // MonoGame try to load as a non-content file
                assetName = TitleContainer.GetFilename(
                    Path.Combine(
                        AppDomain.CurrentDomain.BaseDirectory,
                        RootDirectory,
                        assetName
                    )

B) When this code continues on to ReadRawAsset, replace the TitleContainer.OpenStream calls with File.OpenRead in order to allow the absolute path. OpenRead is all OpenStream really does other than stuff we don't want to do in the non-content situation.

Now, Nick brings up a situation that I didn't consider, which is allowed in XNA (if he's to be believed <g>). That is, if the RootDirectory has been changed to a non-relative path we're also going to get an exception when trying to load the XNB file because our code pipes that straight into TitleContainer.OpenStream.

This will require a separate fix where we check to see if the RootDirectory path is rooted, and just call the local OpenStream if it is:

        protected virtual Stream OpenStream(string assetName)
        {
            Stream stream;
            try
            {
                string assetPath = Path.Combine(RootDirectory, assetName) + ".xnb";
                if (Path.IsPathRooted(RootDirectory))
                {
                    stream = File.OpenRead(assetPath);
                }
                else{
                    stream = TitleContainer.OpenStream(assetPath);
                }

I've added that to my changes and don't immediately see any ill effects. Give me a day to think about it, and I'll submit a patch for discussion.

from fna-mghistory.

Whoops... had assetpath where I meant RootDirectory in last post. Fixed. And fixed where it hid my <g> in the paragraph with Nick. :)

Update: Must stop working on four things at once. Fixed in correct line now.

from fna-mghistory.

Turns out we've got something that might work for everything:

https://github.com/flibitijibibo/MonoGame/blob/monogame-sdl2/MonoGame.Framework/Content/ContentManager.cs#L631

Basically just needs a check for IsFullPath, then we just use this instead of RootDirectory explicitly for places where this is used, avoiding TitleContainer.OpenStream at the same time.

from fna-mghistory.

But.... in my original scenario, I'm not giving Load full paths, I just need the path to be relative to the program directory instead of current.

from fna-mghistory.

Hm, is that accurate to XNA4? I always thought the program directory was the current directory.

from fna-mghistory.

I see no reason a windows user couldn't start a program while they were in another directory, and XNB files are always loaded relative to the program directory, which is why I think we should be doing the same for non-XNB content (since the usual case will be swapping in/out a native file for an XNB).

from fna-mghistory.

Are we not doing that already then? TitleContainer.Location is the program directory, so when IsFullPath is false, we'd just use TitleContainer.Location instead.

from fna-mghistory.

This was the patch I was thinking of:

http://www.flibitijibibo.com/rootDir.txt

Does this work for everything?

EDIT: Forgot about the TitleContainer.OpenStream lines!
EDIT2: Added a GetFilename for the ContentManager.OpenStream.

from fna-mghistory.

Are we not doing that already then?

Ooooh... We almost are. You just made me realize the real problem with the code as is today:

https://github.com/flibitijibibo/MonoGame/blob/monogame-sdl2/MonoGame.Framework/Content/ContentManager.cs#L294

Normalize ends up calling File.Exists(fileName) with various extensions and returns null when none of the variations exist. And, since File.Exists isn't using pathing relative to the program - it will always null out the assetName if you're running it from another directory.

from fna-mghistory.

Cool, see if it can be combined with the above patch as well. This patch seems to fix the absolute RootDirectory as well, so we may have both resolved after that.

from fna-mghistory.

Sure thing. I will take care of it right after I get off work!

from fna-mghistory.

We just finished the Content/ FNA cleanup task, so I've updated the rootDir.txt diff.

from fna-mghistory.

Lol... you and me both. I noticed the conflict and pushed an updated version to my fork. haha. Was just looking it over to make sure it takes care of the issue Nick mentioned, and not just the scenario I ran into.

from fna-mghistory.