Page MenuHome
Differential D1071

Ptex rendering/painting
Needs RevisionPublic
Actions

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

Details

Reviewers
Sergey Sharybin (sergey)
Antonis Ryakiotakis (psy-fi)
Commits
rB87f3139a4140: Temporary hack to ensure packed image pointer
rB57ee82d61b5e: Hacky fix for saving texpaint ptex changes
rB8e3449cd1a13: Add Ptex mesh operators
rB99a40c59b24d: Add initial Ptex support to texture paint mode
rB990a2748c4c1: Add Ptex to mesh-data UI
rB847e90c531e7: Allow ptex node to be a texpaint slot
rBad1293f636fa: Update Blend file saving/loading for Ptex
rB6ab32269205e: Add Ptex UV interpolation to subsurf modifier
rB7bc964b4f994: Add Ptex infrastructure to GPU module
rB29f3e1374b3b: Add support for GPU drawing of Ptex textures to image node
rB964285c25e29: Add Ptex support to Cycles image node, also add Ptex node
rBc50694060db8: Add Ptex shader node
rB4d7e0d3cd733: Add loading of Ptex UVs and layer data in Cycles
rBf59d0f9f3ca5: Update DerivedMesh for Ptex drawing/rendering
rB3625fb6bfd45: RNA updates for Ptex
rBfd923fcefba9: Tesselation for Ptex loop layer
rB677c606dcb61: Add new CustomData layers for Ptex
rBf4317a97891a: Move Cycles' Ptex attribute from vertices to corners
rBbc5fa16ddbfb: Add basic Ptex reading via OIIO to ImBuf
rBb1cdf58286b5: Add API for packing Ptex faces into a single large image
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

Event Timeline

Nicholas Bishop (nicholasbishop) updated this revision to Diff 3386.Feb 6 2015, 5:32 PM
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) updated this object.
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
Nicholas Bishop (nicholasbishop) updated this object.
Nicholas Bishop (nicholasbishop) added reviewers: Antonis Ryakiotakis (psy-fi), Sergey Sharybin (sergey), Campbell Barton (campbellbarton).
Closed by commit rBb1cdf58286b5: Add API for packing Ptex faces into a single large image (authored by Nicholas Bishop (nicholasbishop)). · Explain WhyFeb 6 2015, 5:35 PM
This revision was automatically updated to reflect the committed changes.
Nicholas Bishop (nicholasbishop) reopened this revision.Feb 6 2015, 5:46 PM

Reopening, silly phabricator.

Ronny G (nutel) added a subscriber: Ronny G (nutel).Feb 6 2015, 6:43 PM
Nicholas Bishop (nicholasbishop) added a comment.Feb 8 2015, 3:55 PM

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.

Nicholas Bishop (nicholasbishop) updated this revision to Diff 3420.Feb 9 2015, 12:14 PM

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
Antonis Ryakiotakis (psy-fi) edited edge metadata.Feb 9 2015, 4:10 PM

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 ↗(On Diff #3420)

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

source/blender/blenkernel/intern/subsurf_ccg.c
3847 ↗(On Diff #3420)

Still needed?

source/blender/editors/sculpt_paint/paint_image_proj.c
3659 ↗(On Diff #3420)

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 ↗(On Diff #3420)

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 ↗(On Diff #3420)

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

Antonis Ryakiotakis (psy-fi) requested changes to this revision.Feb 9 2015, 4:10 PM
Antonis Ryakiotakis (psy-fi) edited edge metadata.
This revision now requires changes to proceed.Feb 9 2015, 4:10 PM
ronan ducluzeau (zeauro) added a subscriber: ronan ducluzeau (zeauro).EditedFeb 9 2015, 4:48 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 ↗(On Diff #3420)

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

extern/ptex/bpx_c_api.cpp
13

Ouch.. What's this is for?

23

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

intern/cycles/CMakeLists.txt
134 ↗(On Diff #3420)

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 ↗(On Diff #3420)

What spi means?

intern/cycles/blender/blender_session.cpp
104 ↗(On Diff #3420)

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

958 ↗(On Diff #3420)

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

intern/cycles/blender/blender_shader.cpp
599 ↗(On Diff #3420)

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

intern/cycles/kernel/kernel_types.h
537 ↗(On Diff #3420)

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

intern/cycles/kernel/osl/osl_services.cpp
53 ↗(On Diff #3420)

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 ↗(On Diff #3420)

What all this is about? =\

402 ↗(On Diff #3420)

Some magic bits..

408 ↗(On Diff #3420)

...And some more magic numbers...

430 ↗(On Diff #3420)

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

Can it be cleaned up?

intern/cycles/render/image.cpp
31 ↗(On Diff #3420)

Would suggest using indentation of nested preprocessor:

#if FOO
#  include "bar"
#endif
805 ↗(On Diff #3420)

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 ↗(On Diff #3420)

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 ↗(On Diff #3420)

Can't this flags be passed via SVM nodes?

source/blender/blenkernel/intern/bke_ptex.c
1 ↗(On Diff #3420)

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 ↗(On Diff #3420)

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 ↗(On Diff #3420)

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

379 ↗(On Diff #3420)

Would need to ask @Campbell Barton (campbellbarton).

source/blender/editors/sculpt_paint/paint_image_proj.c
3743 ↗(On Diff #3420)

Interesting, is it part of other fix or so?

Erick Tukuniata (erickblender) added a subscriber: Erick Tukuniata (erickblender).Mar 13 2015, 12:48 PM
Greg Zaal (gregzaal) added a subscriber: Greg Zaal (gregzaal).Jul 25 2016, 5:29 PM

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

Campbell Barton (campbellbarton) removed a reviewer: Campbell Barton (campbellbarton).Aug 11 2016, 6:12 AM

Revision Contents
Changeset List

PathSize
build_files/
cmake/
1 line
extern/
4 lines
2 lines
ptex/
151 lines
38 lines
8 lines
1260 lines
144 lines
intern/
cycles/
4 lines

Diff 3387

View Options

build_files/cmake/macros.cmake

Loading...
View Options

extern/CMakeLists.txt

Loading...
View Options

extern/SConscript

Loading...
View Options

extern/ptex/BPX_pack.h

Loading...
View Options

extern/ptex/CMakeLists.txt

Loading...
View Options

extern/ptex/SConscript

Loading...
View Options

extern/ptex/bpx_c_api.cpp

Loading...
View Options

extern/ptex/ptex_packed_layout.h

Loading...
View Options

intern/cycles/SConscript

Loading...