Page MenuHome

Ptex rendering/painting
Needs RevisionPublic

Authored by Nicholas Bishop (nicholasbishop) on Feb 6 2015, 5:32 PM.

Details

Summary

First pass at supporting Ptex in Blender. Code is not at all in a
finished state, currently there are over 100 "TODO" comments and
probably should be even more :) But I think it's far enough that it
can get some productive review about overall design.

See individual commits for a bit more detail, and also have a look at
documentation on the wiki here:
http://wiki.blender.org/index.php/User:Nicholasbishop/Ptex#Implementation

Diff Detail

Repository
rB Blender
Branch
cycles-ptex-49

Event Timeline

Nicholas Bishop (nicholasbishop) retitled this revision from to First pass at supporting Ptex in Blender. Code is not at all in a finished state, currently there are over 100 "TODO" comments and probably should be even more :) But I think it's far enough that it can get some productive review about overall....
Nicholas Bishop (nicholasbishop) retitled this revision from First pass at supporting Ptex in Blender. Code is not at all in a finished state, currently there are over 100 "TODO" comments and probably should be even more :) But I think it's far enough that it can get some productive review about overall... to Ptex rendering/painting.Feb 6 2015, 5:34 PM
This revision was automatically updated to reflect the committed changes.

Reopening, silly phabricator.

Juicyfruit noted in IRC that part of the diff isn't showing up here. I have no idea why... the cycles-ptex-49 branch in git should be fully up to date, however.

Syncing diff with the updated branch

/me cross fingers that I'm doing correct arc options

  • Compilation fix for game engine
  • Fix linker error in blenderplayer
  • Merge remote-tracking branch 'origin/master' into cycles-ptex-49
  • Minor code cleanups for Ptex
  • Minor code cleanups for Ptex
  • Move some of the Ptex code behind a WITH_PTEX ifdef
  • Merge remote-tracking branch 'origin/master' into cycles-ptex-49
  • Minor includes cleanup in bke_ptex.c
  • Naming cleanup: better consistency in extern/ptex
  • Add missing OIIO include dirs for CMake in extern/ptex
  • Improve performance of mesh iteration in ptex_pack_loops
  • Ptex+Cuda compilation fix
  • Merge remote-tracking branch 'origin/master' into cycles-ptex-49
  • Minor TODO fix, rename PtexDataType -> MPtexDataType
  • Ptex+Cuda fix, use kernel_assert instead of assert
  • Potential Ptex+Cuda fix, adjust number of GPU texture slots
  • Bring back __tex_image_150 on DingTo's advice
  • Ptex+Cuda: update the svm_image_texture switch
  • Fix crash in Ptex+Cycles
  • Make default Ptex layer a better color than flat black
  • Ptex SCons include tweaks
  • Ptex: fix memory leak from subsurf cache
  • Use file browser instead of hardcoded path in Ptex import operator
  • Add Ptex extension (ptx) to imb_ext_image array
  • Use filename as the new layer name in Ptex import operator
  • Set "*.ptx" filter glob for Ptex import operator
  • Select new layer after importing a Ptex layer
  • Merge remote-tracking branch 'origin/master' into cycles-ptex-49
  • Ptex+OSL compilation fix, change ifdef guard for older Ptex code
  • Merge remote-tracking branch 'origin/master' into cycles-ptex-49
  • Fix ImBuf memory leak in Ptex

Preliminary review with some tidbits. I can't say a lot about cycles, but I think texpaint could use a little more refinement - I didn't see if there are UI for switching between ptex/texture workflows yet.

I will be trying a compile later to see how it works and feels in general.

intern/cycles/render/nodes.cpp
558

Are those relevant? AFAIK ptex has its own generated coordinates always..

source/blender/blenkernel/intern/subsurf_ccg.c
3847

Still needed?

source/blender/editors/sculpt_paint/paint_image_proj.c
3659

Probably you can reuse the original mtface pointer here, it doesn't really make sense to keep both since we either paint on a regular slot or a ptex slot.

source/blender/gpu/intern/gpu_codegen.c
838

Generally, having the Material system depend on the object is not very nice (makes reusing the materials harder and requires rebinding each time we change the object)

source/blender/makesdna/DNA_customdata_types.h
126

If the loop data structures don't make sense outside ptex then combining them to one struct might make more sense.

Antony Riakiotakis (psy-fi) requested changes to this revision.Feb 9 2015, 4:10 PM
Antony Riakiotakis (psy-fi) edited edge metadata.
This revision now requires changes to proceed.Feb 9 2015, 4:10 PM

Currently, texture creation in texture paint mode creates corresponding nodes in material and adding simple UVs can be done by pressing one button.
And user can directly paint on it.

In ptex branch, user have to create Ptex layer, to create, to connect the Ptex Texture node to a diffuse node and to fill it with Ptex Layer name before being able to paint.
Ptex layer display in 3D Viewport is only possible in Material View. So, it is sensible to material settings. There is no single Ptex Layer display as for texture.
It seems that painting of ptex layer and image texture is not supported.
Active Material Slot cannot be changed. You cannot switch from a Ptex layer to an UVmap texture.

No matter what Ptex layer node is selected in material, you are painting on active slot in mesh properties.

Sergey Sharybin (sergey) requested changes to this revision.Feb 15 2015, 1:26 PM
Sergey Sharybin (sergey) edited edge metadata.

Initial set of comments. Was mainly looking into Cycles side and only briefly scrolled through rest of the code.

There are also quite enough of TODOs marked by you which probably will be easier for you to grep through the sources.

extern/ptex/BPX_packed_layout.h
137

Tip: use wither _ suffix (matching libmv/ceres) or m_ prefix (matching compositor) for private members.

extern/ptex/bpx_c_api.cpp
12

Ouch.. What's this is for?

22

For master would suggest dropping dead code (or maybe comment that it'll be used later?)

intern/cycles/CMakeLists.txt
134

Would need to be adjusted for standalone repository, but that's easier to worry about after the merge.

intern/cycles/SConscript
88

Can this two blocks be de-duplicated?

intern/cycles/blender/blender_mesh.cpp
418

What spi means?

intern/cycles/blender/blender_session.cpp
104

Just thinking loud.. Did anyone look into a way to give placeholders more reasonable names?

958

Seems to be really bad idea. Can't it be handled properly in NA level?

intern/cycles/blender/blender_shader.cpp
599

Seems it's actually the same as adding image node with .ptx file?

intern/cycles/kernel/kernel_types.h
537

Since this is pure run=time code, what about grouping PTex attributes together?

intern/cycles/kernel/osl/osl_services.cpp
53

Hrm. Seems you can enable PTex for SVM and keep it disabled for OSL? Is there really need to such a granularity?

intern/cycles/kernel/svm/svm_image.h
385

What all this is about? =\

402

Some magic bits..

408

...And some more magic numbers...

430

So far this is biggest concern in the kernel code..

Can it be cleaned up?

intern/cycles/render/image.cpp
31

Would suggest using indentation of nested preprocessor:

#if FOO
#  include "bar"
#endif
805

Now i kinda see why you passed dscene in there. Perhaps name would need to be adjusted.

Would need to have a closer look to fully understand the code. o have feeling it could simplified/cleaned but cant' really give actual feedback yet :)

intern/cycles/render/image.h
105

This sounds a bit weird. File I/O shouldn't really depend on device and device-specific code is to be part of device_* functions.

Otherwise i smell some headache for persistent data case..

intern/cycles/render/nodes.cpp
472

Can't this flags be passed via SVM nodes?

source/blender/blenkernel/intern/bke_ptex.c
1

Name is a bit weird. We've got BLI_ prefix for files in blenlib, or we just don't put module into the name.

So would suggest either BKE_ptex.c (which would be first file with such prefix in BKE) or simply ptex.c.

139

Oh geez. That's becoming crazy and that's why i like to put todo's description when just writing them. Helping to understand how important the thing is and if you're keeping the same thing to be changed in mind.

So you'll need to go over the TODOs and double-check if they're stoppers from your POV or not..

337

Ok question, what if we'll want to extend those structures? :)

379
source/blender/editors/sculpt_paint/paint_image_proj.c
3743

Interesting, is it part of other fix or so?

What's the status on this Ptex work? Sorry if I missed another thread where conclusions were made.