facebookarchive / react-360

Create amazing 360 and VR content using React
https://facebook.github.io/react-360
Other
8.73k stars 1.23k forks source link

Bump maps and options not read without extra spaces #349

Closed jgwinner closed 7 years ago

jgwinner commented 7 years ago

Description

Bug - code indexes .MTL file options incorrectly for bump maps

Expected behavior

When a .MTL file that's part of an .OBJ file is read, it should parse lines like:

bump -bm 0.01 1_New_Graph_Height.jpg

Actual behavior

It skips the -bm statement unless you cram a lot of spaces in there (which most modelers won't do of course)

won't be parsed

bump -bm 0.01 1_New_Graph_Height.jpg

will be parsed

bump -bm 0.01 1_New_Graph_Height.jpg

It also means if you don't have the -bm option or the spaces, it won't read the texture file.

Reproduction

Edit any .MTL file to include bump maps

Solution

On line 441 of MTLParser.js, it comares the line to 'bump' || map_bump

After that, it starts parsing at the 6th character. This should be 4 or 8.

let isBump = false; let bumpStart = 4 ;

if (line.substr(index, 4).toLowerCase() === 'bump') { console.log("readLine: Reading bump"); //bumpStart 4 by default isBump=true; } if ( line.substr(index, 8).toLowerCase() === 'map_bump') { console.log("readLine: Reading map_bump" ); isBump=true; bumpStart=8; } if (isBump) { const tex = readTextureOptions(line.substr(index + bumpStart).trim()); if (tex.file) { console.log("readLine: read bump options" + tex.file); if (!latest.textureMap) { latest.textureMap = {}; } if (!tex.options.imfchan) { tex.options.imfchan = 'l'; } latest.textureMap.bump = tex; } }

(Or just duplicate the block like the other sections, with one check for 'bump' and start at 4 and one check for map_bump and start at 8.

Additional Information

jgwinner commented 7 years ago

Whoops, sorry for the debugging console lines.

Of course, you have to be careful with bump maps - would be nice if the underlying renderer took parallax into account, but that's a hit or miss thing. I know you guys don't like it, but we should allow it, and let the artist / scene developer evaluate the tradeoffs. Displacement may be better, but I noticed there's a bug in Displacement as well.

if (line.substr(index, 4).toLowerCase() === 'disp') { const tex = readTextureOptions(line.substr(index + 6).trim());

should be const tex = readTextureOptions(line.substr(index + 4).trim());

I also noted that the GearVR browser looks TERRIBLE with bump maps. However, if you exit out of the browser, then go into Carmel, exit out of that, and back into the browser, it looks GREAT.

Apparently Carmel does something persistent with the rendering. This is not a ReactVR bug of course.

I haven't checked my Vive yet - for development usually the Gear is faster, so I've been using that. Will update if it looks terrible as well.

This is a browser issue though - I didn't notice any stereoscopic issues once the aliasing was better, at least in my scene, and it looked great.

jgwinner commented 7 years ago

Er, 5. Geez.

andrewimm commented 7 years ago

I think this got fixed in 5daf7b3b7ead0d13c40637b988560fe9f0e0edcf, shipped in 2.0.0

jgwinner commented 7 years ago

Looks great!