Wavefront loader path handling missing

You discovered a bug in the engine, and you are sure that it is not a problem of your code? Just post it in here. Please read the bug posting guidelines first.
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Wavefront loader path handling missing

Post by robmar »

The CObjMeshFileLoader.cpp code is meant to handle externally user-defined paths, but fails to strip any MTL set path from the texture name.

Line 435:-

Code: Select all

        if ( texnameWithUserPath.size() )
        {
            texname = FileSystem->getFileBasename( texname );   // Strip MTL preset path if exists from name. Robmar mode
 
            texnameWithUserPath += texname;
        }
 
The code to strip the filename could also be faster:-

Code: Select all

 
//! returns the base part of a filename, i.e. all except for the directory
//! part. If no directory path is prefixed, the full name is returned.
io::path CFileSystem::getFileBasename(const io::path& filename, bool keepExtension) const
{
    if (keepExtension)
       {
    u32 size = filename.size();     // Robmar mod
    int i;
    for ( i = (int)size-1; i >= 0 && filename[i] != '/' && filename[i] != '\\'; i-- ) ;      // Find last slash
    if ( i >= 0 )
    {
        return filename.subString( i+1, size - (i+1) );
    }
    else
        return filename;
    }
    else
    {  
    // find last forward or backslash
    s32 lastSlash = filename.findLast('/');
    const s32 lastBackSlash = filename.findLast('\\');
    lastSlash = core::max_(lastSlash, lastBackSlash);
 
    // get number of chars after last dot
    s32 end = 0;
    if (!keepExtension)
    {
        // take care to search only after last slash to check only for
        // dots in the filename
        end = filename.findLast('.');
        if (end == -1 || end < lastSlash)
            end=0;
        else
            end = filename.size()-end;
    }
 
    if ((u32)lastSlash < filename.size())
        return filename.subString(lastSlash+1, filename.size()-lastSlash-1-end);
    else if (end != 0)
        return filename.subString(0, filename.size()-end);
    else
        return filename;
    }
}
 
CuteAlien
Admin
Posts: 9734
Joined: Mon Mar 06, 2006 2:25 pm
Location: Tübingen, Germany
Contact:

Re: Wavefront loader path handling missing

Post by CuteAlien »

Can you please post an example .mtl file with a corresponding material so we can see how that would look like for this case?

About the getFileBasename changes, maybe you heard of the proverb from Knuth before: "We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil". You avoid a function like findLast and instead re-implementing the same in a loop. But that means you have now twice the code that has to be maintained and twice the amount of code which can contain bugs. I wonder for example what happens when filename is compiled with wchar_t's. Also the code is now longer and harder to read. This kind of optimization only makes sense if you have concrete cases where you profiled the code and noticed a bottleneck (we have that in other places - for example in xml-writing where the current way of string-manipulations clearly is a bottleneck).
IRC: #irrlicht on irc.libera.chat
Code snippet repository: https://github.com/mzeilfelder/irr-playground-micha
Free racer made with Irrlicht: http://www.irrgheist.com/hcraftsource.htm
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Re: Wavefront loader path handling missing

Post by robmar »

yeah I agree with you, it was just that the code looked really heavy, and simply scanning back from the end looks like the way to do it.

findlast maybe should been tuned up, but its not a bottleneck area. that said lots of inefficiencies can add up, not to mention that if code like that is there in a non critical function, where else will it be hiding if no one cares?!

this is a typical mtl with partial path preset:-

# 3ds Max Wavefront OBJ Exporter v0.97b - (c)2007 guruware
# File Created: 19.01.2013 22:34:44

newmtl wire_022022022
Ns 32.0000
Ni 1.5000
d 1.0000
Tr 0.0000
Tf 1.0000 1.0000 1.0000
illum 2
Ka 0.0863 0.0863 0.0863
Kd 0.0863 0.0863 0.0863
Ks 0.3500 0.3500 0.3500
Ke 0.0000 0.0000 0.0000
map_Ka maps\MRAMOR6X6.jpg
map_Kd maps\MRAMOR6X6.jpg
hybrid
Admin
Posts: 14143
Joined: Wed Apr 19, 2006 9:20 pm
Location: Oldenburg(Oldb), Germany
Contact:

Re: Wavefront loader path handling missing

Post by hybrid »

The .mtl loader part tries to load from a) OBJ_PATH_NAME/texname, b) texname c) obj_rel_path/texname
where texname is the name from the mtl file, OBJ_PATH_NAME is a global variable configurable by the user and obj_rel_path is the path of the .obj file. This is what is usually done in all mesh file loaders.
CuteAlien
Admin
Posts: 9734
Joined: Mon Mar 06, 2006 2:25 pm
Location: Tübingen, Germany
Contact:

Re: Wavefront loader path handling missing

Post by CuteAlien »

@robmar you mean the the "maps\" part should be removed? But that seems wrong - if there is a folder name it should be regarded (otherwise working with different sub-folders will become rather difficult). Wouldn't it be a better solution in your case to keep your textures in the "maps" folder?
IRC: #irrlicht on irc.libera.chat
Code snippet repository: https://github.com/mzeilfelder/irr-playground-micha
Free racer made with Irrlicht: http://www.irrgheist.com/hcraftsource.htm
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Re: Wavefront loader path handling missing

Post by robmar »

Yeah but there is a switch in irrlicht that lets the user specify the path.

What I am saying is that if that switch is on, then any embedded paths MUST be stripped before adding on the user set path.
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Re: Wavefront loader path handling missing

Post by robmar »

So if texnameWithUserPath is used, just discard any embedded path immediately because its never ever going to be correct.
CuteAlien
Admin
Posts: 9734
Joined: Mon Mar 06, 2006 2:25 pm
Location: Tübingen, Germany
Contact:

Re: Wavefront loader path handling missing

Post by CuteAlien »

I still don't think that would be correct. You set a base-path, but relative paths should still not be ignored. Ignoring those never seems to be correct to me.
IRC: #irrlicht on irc.libera.chat
Code snippet repository: https://github.com/mzeilfelder/irr-playground-micha
Free racer made with Irrlicht: http://www.irrgheist.com/hcraftsource.htm
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Re: Wavefront loader path handling missing

Post by robmar »

Okay, logic is this:-

1. User does not set a path in Irrlicht, loader loads as specified in MTL- okay?

2. User sets a path in Irrlicht, loader DUMPs path in MTL are in place uses path set by user - okay?

End. Stop.
CuteAlien
Admin
Posts: 9734
Joined: Mon Mar 06, 2006 2:25 pm
Location: Tübingen, Germany
Contact:

Re: Wavefront loader path handling missing

Post by CuteAlien »

I've not written that code, but the way it works is pretty much how I expected it to work when I first run into it. It's just working like in other software where you can set a base-path. Like when adding a path to your systems PATH variable or when you add path's in your IDE for libs or include folders and so on. Note that this code was written before we had the more general IFileSystem::addFileArchive which can pretty much replace this completely by now I suppose. Though having now both ... no idea - maybe having what you want here might even make sense. But it would break old software using it the way it's supposed to work now, so if we ever add that we should use a new parameter name for it in that case.
IRC: #irrlicht on irc.libera.chat
Code snippet repository: https://github.com/mzeilfelder/irr-playground-micha
Free racer made with Irrlicht: http://www.irrgheist.com/hcraftsource.htm
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Re: Wavefront loader path handling missing

Post by robmar »

n o no non nicht! read my lips please!

MTL file texture = f:\maple\sub\321\frgnimage.jpg

Okay?

User sets Irrlicht path override in Irrlicht to path = c:\myfrnimage\mynicefolder\f:\maple\sub\321\frgnimage.jpg

Ummmm... I must have missed something, which is possible as my brain is a bit burned, pardon the pun (good username - bitburned!), anyway,
as I see it any path in the mtl needs to be stripped before the user path is concatenated.

but then... maybe I have a hole in my bitbrain?! :)

Driver code for irrlicht concatenates to produce :- c:\myfrnimage\mynicefolder\
CuteAlien
Admin
Posts: 9734
Joined: Mon Mar 06, 2006 2:25 pm
Location: Tübingen, Germany
Contact:

Re: Wavefront loader path handling missing

Post by CuteAlien »

Hm, ok, so far we were talking about relative paths. Now this is about absolute paths. Never tried that but what I would expect is that absolute paths are not merged at all (again similar to the way this is handled for example with include paths by compilers). You shouldn't use absolute paths if you don't want absolute paths. If those are merged like you describe it then that is indeed a bug. Still stripping paths should not happen here, except maybe if this is made an own option. It's not downward compatible (it would break existing application code) and simply not what I think people would usually expect (because it's not how base-paths work in other applications).

I can understand that it's a feature you might like to see. Not sure it's a good idea ever using such a feature, but I also wouldn't care if it's added (although a little tricky where to put that flag as loader-interfaces are not accessible, so either has to be coded for all loaders or yet another ugly global attribute used which I would dislike as we already have trouble with having too many of those and I rather think currently about how to remove some of them again). And merging absolute paths is a bug if it happens and should be fixed. But I'm definitely against stripping paths from mtl just like that.

edit: Or to think about this in another way. If paths would be stripped then it would no longer be possible to use more than one folder for your textures when setting a path (as this is one global variable). You would force users to put all textures in a single folder!
IRC: #irrlicht on irc.libera.chat
Code snippet repository: https://github.com/mzeilfelder/irr-playground-micha
Free racer made with Irrlicht: http://www.irrgheist.com/hcraftsource.htm
robmar
Posts: 1125
Joined: Sun Aug 14, 2011 11:30 pm

Re: Wavefront loader path handling missing

Post by robmar »

Phew, how can this the so difficult to explain!!

I need to use a fixed relative path to the textures for wavefront obj files because there are millions of nice useful models on the web, nearly all of which were created on a specific computer which embedded the path into the MTL.

So if people want to benefit from all those models, they need to use the Irrlicht support control to allow a specific path to be used.

Is that agreed? I mean if not I give up right now and will desist from continuing this thread think that you are having a laugh with me!! :P
hendu
Posts: 2600
Joined: Sat Dec 18, 2010 12:53 pm

Re: Wavefront loader path handling missing

Post by hendu »

Uh, why are you not clearing up the paths of those random models when you add them to your project? Why are you not doing it once, and instead want it to happen at runtime?
CuteAlien
Admin
Posts: 9734
Joined: Mon Mar 06, 2006 2:25 pm
Location: Tübingen, Germany
Contact:

Re: Wavefront loader path handling missing

Post by CuteAlien »

I get what you want - but I think I also explained the reasons why it is as it is in Irrlicht sufficiently by now.
IRC: #irrlicht on irc.libera.chat
Code snippet repository: https://github.com/mzeilfelder/irr-playground-micha
Free racer made with Irrlicht: http://www.irrgheist.com/hcraftsource.htm
Post Reply