Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#1497 closed enhancement (fixed)

[PATCH] Support for multiple UV sets per model

Reported by: myconid Owned by: myconid
Priority: Nice to Have Milestone: Alpha 11
Component: Core engine Keywords: patch texturing
Cc: Patch:

Description

Collada files can support more than one UV coordinate set, however 0ad's .PMD version 3 files are unable to store them so they are currently ignored.

0ad's models use overlapping UVs to make optimal use of UV space on their base textures. While this is beneficial, it makes it impossible to implement more advanced texturing techniques, such as lightmaps (e.g., AO maps), decal maps and so on.

The attached patch upgrades the engine to .PMD version 4, which introduces support for multiple UV sets.

Attachments (4)

multicoord.diff (17.4 KB ) - added by myconid 12 years ago.
multicoord-final.diff (18.3 KB ) - added by myconid 12 years ago.
multicoord-final2.diff (18.1 KB ) - added by myconid 12 years ago.
multicoord-final3.diff (18.7 KB ) - added by myconid 12 years ago.

Download all attachments as: .zip

Change History (28)

by myconid, 12 years ago

Attachment: multicoord.diff added

comment:1 by Kieran P, 12 years ago

Keywords: review added
Milestone: BacklogAlpha 11
Summary: Support for multiple UV sets per model[PATCH] Support for multiple UV sets per model

in reply to:  description comment:2 by historic_bruno, 12 years ago

Replying to myconid:

Collada files can support more than one UV coordinate set, however 0ad's .PMD version 3 files are unable to store them so they are currently ignored.

Hopefully Blender correctly exports models with multiple UV coordinate sets? Preferably a recent version of Blender (2.6+) and in a way which works with our importer, since Blender's COLLADA support is sketchy :( I'm curious if you tested that? And that it doesn't break existing model import/rendering. Max exporter would be nice to test if we can find someone actively using it.

Also how does this tie in with pre-generating AO textures in the engine, would models need to be exported with multiple UV sets from the beginning or can they be created on demand?

comment:3 by myconid, 12 years ago

It's been tested with Blender 2.63 and it works great. See the usual forum thread for a pic and some test files. Blender's exporter must have come a long way lately, because the file with UVs is actually smaller than the original (1.3M -> 1.1M)!

This is still needed if you're generating the AO textures in the engine. With AO (and lightmaps in general) you must have UVs that don't overlap, since each surface needs to be lit independently of the others.

Models need to be exported like this from the beginning, if you want to apply lightmaps to them (if you don't, then it doesn't matter if the second UVs are missing). Lightmaps are a much bigger topic than just AO, and all lightmaps share the same UV set. We want lightmaps to be both loadable from textures (created by artists) and possible to compute in the engine. As a result, the only way that UVs can be consistent in all these cases is if they are stored in the model.

In Blender, creating a second UV set automatically and rendering AO is as simple as a few mouse clicks. There seems to be a way to write a Blender script to do these things for us...

in reply to:  3 ; comment:4 by historic_bruno, 12 years ago

Replying to myconid:

Models need to be exported like this from the beginning, if you want to apply lightmaps to them

OK, I just wanted to confirm that this does require re-exporting and some extra work on the part of the artists :) I'll see if we have most or all of the original .blend/.max files, otherwise we'd need to import the DAEs first...

in reply to:  4 comment:5 by myconid, 12 years ago

Replying to historic_bruno:

OK, I just wanted to confirm that this does require re-exporting and some extra work on the part of the artists :) I'll see if we have most or all of the original .blend/.max files, otherwise we'd need to import the DAEs first...

We can import the DAEs, though Blender seems to have some issues with missing texture files that require us to go in the xml and remove the references manually. I think we should use Autodesk's free utility to convert DAE -> FBX -> OBJ and import the OBJ into Blender, as that has no issues.

in reply to:  3 comment:6 by historic_bruno, 12 years ago

Replying to myconid:

In Blender, creating a second UV set automatically and rendering AO is as simple as a few mouse clicks. There seems to be a way to write a Blender script to do these things for us...

If Blender can do this automatically why can't we? Is it very complicated, that we couldn't add it to our model importing? (if it turns out to be simple, I don't see why we couldn't do this ourselves and save having to import and re-export a bunch of models from Blender)

Maybe there's an advantage to doing it in Blender, in that it preserves the multiple UV sets in the DAE.

comment:7 by myconid, 12 years ago

I doubt it's too complicated, though the same UVs that you'd generate in the importer also need to be available to the artists outside the game when they're creating their lightmaps. Either you write a DAE exporter to save out the UVs you generate, or you let the artists generate the UVs and we import them into the game. The patch provides the latter, IMHO better option.

comment:8 by historic_bruno, 12 years ago

OK we can go with that for now. If it proves to be too much work for artists, maybe we can create a second UV set for models imported with only one (or older PMD version than whatever we are currently) which would allow a proposed engine AO calculation for all models even if they haven't been updated to multiple UV sets.

Anyway that would seem to build on this patch, rather than replace it.

comment:9 by historic_bruno, 12 years ago

There's a build error btw, I checked and it's still in your git repo. source/collada/PMDConvert.cpp fails in a few places because vertexCount is referenced in an assert() before it's defined. Easy fix :)

in reply to:  9 comment:10 by historic_bruno, 12 years ago

Replying to historic_bruno:

There's a build error btw, I checked and it's still in your git repo. source/collada/PMDConvert.cpp fails in a few places because vertexCount is referenced in an assert() before it's defined. Easy fix :)

I should point out you won't notice this error unless you build in debug config, as the assert() does nothing in release builds, so it's easy to overlook.

by myconid, 12 years ago

Attachment: multicoord-final.diff added

comment:11 by myconid, 12 years ago

Submitting multicoord-final.diff for review. The assert error has been corrected (I believe).

in reply to:  11 comment:12 by historic_bruno, 12 years ago

Replying to myconid:

Submitting multicoord-final.diff for review. The assert error has been corrected (I believe).

I forgot to ask last time, what's the motivation behind commenting out the stream flags stuff? (in ShaderProgram.cpp, HWLightingModelRenderer.cpp, etc.) If it's something totally incompatible with the new changes, we should remove it, otherwise we should make it work.

Are these patches ready to be committed after review?

by myconid, 12 years ago

Attachment: multicoord-final2.diff added

comment:13 by myconid, 12 years ago

Fixed stream thingy. It was an omission, no good reason for it.

Ready for review.

comment:14 by Jonathan Waller, 12 years ago

In GeomReindex.cpp

You modify the struct to have std::vector<float> uvs. It would be nicer to create a uv struct {float u, float v} and have a vector of them.

In bool operator==(const VertexData& a, const VertexData& b) you compare uvs using ==, if I understand this correctly this isn't going to use the similar() function so the tolerance for the comparison will be lost.

In PMDConvert.cpp it would be nice to have a slightly expanded comment for line 507.

I haven't finished looking through the whole patch yet, I will need to sleep before I can do any more. It applies cleanly compiles and runs fine for me which is good.

comment:15 by myconid, 12 years ago

Thanks. Since these are really minor changes I'll update the patch when you're through the whole thing, if that's alright.

comment:16 by Jonathan Waller, 12 years ago

I've had a look through the rest. It looks like a nice patch.

InstancingModelRenderer.cpp 56: VertexArray::Attribute m_UVs[5]; I wasn't entirely sure what the [5] meant in this context. My best guess was that it makes an array of 5 possible m_UVs to be used. If so then there needs to be some form of error checking in case the number of UV's >5.

comment:17 by myconid, 12 years ago

Whoops! :/ That line was replaced in a later commit on git, which I forgot to include in the patch (how typical of me).

Alright, I'll get you the updated patch once I get home.

by myconid, 12 years ago

Attachment: multicoord-final3.diff added

comment:18 by myconid, 12 years ago

Done!

comment:19 by Jonathan Waller, 12 years ago

Resolution: fixed
Status: newclosed

In 12183:

Add support for multiple UVs and data driven texture loading. From myconid's patches. Fixes #1493 and fixes #1497.

comment:20 by myconid, 12 years ago

Looks like you committed the older version to svn. Was that intentional?

comment:21 by Kieran P, 12 years ago

Uh oh! Can you provide a patch with the changes missed? I'll find someone to commit it ASAP.

Last edited 12 years ago by Kieran P (previous) (diff)

comment:22 by ben, 12 years ago

In 12185:

Updates to myconid's most recent multiple UV set patch. Refs #1497

comment:23 by historic_bruno, 12 years ago

Keywords: review removed

comment:24 by ben, 12 years ago

In 12205:

Fixes build error left out of r12185. Refs #1497

Note: See TracTickets for help on using tickets.