Page MenuHome

LANPR line rendering (summer of code)
Needs ReviewPublic

Authored by YimingWu (NicksBest) on Aug 9 2019, 3:28 AM.
Tokens
"Love" token, awarded by samytichadou."Love" token, awarded by Way."Love" token, awarded by kynu."Like" token, awarded by HARDNAX."Manufacturing Defect?" token, awarded by momotron2000."Love" token, awarded by johantri.

Details

Summary

LANPR Patch

This is the patch for soc-2019-npr branch. Now modified as containing only LANPR changes

This patch doesn't include the following:

  • GPencil modifiers.
  • Smooth contour modifier.
  • SVG.
  • Affected UI scripts.
  • Freestyle changes.

Those above will be submitted in other diffs.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
release/scripts/startup/bl_operators/lanpr.py
34

Objects are not uniquely identified by name, there can be multiple with the same name from different libraries.

Why is this operator needed, is it not possible to either ignore the object transform or take it into account? Requiring objects to be at the origin is not convenient for users, they get in the way and overlap each other.

release/scripts/startup/bl_ui/properties_data_modifier.py
23

Unnecessary import.

404–406

You could have a warning here if the LANPR engine is active, but I think ignore_lanpr should be removed. Users who just want to modify normals can use Autosmooth.

release/scripts/startup/bl_ui/properties_lanpr.py
2

Does this really need its own tab in the properties editor? Why not add panels to the object properties tab?

release/scripts/startup/bl_ui/properties_render.py
1096

Options -> Settings

source/blender/editors/include/ED_lanpr.h
707

This whole implementation of own memory allocation, list, math functions and others should not exist. You should use the existing functions, and if needed extend them.

814

This header should have just public API functions from the LANPR module (ED_lanpr). Now it seems to be a mix of public and private.

source/blender/makesrna/intern/rna_collection.c
373

The description should explain what this does at a user level. Does it mean "Render lines with LANPR for objects in the collection"?

374

collction -> collection

411

object who -> objects that

Description is unclear though, which rule? What does it mean not to follow the rule?

415

Don't abbreviate Grease Pencil as GPencil in descriptions.

422

I think this should be a name string rather than an integer.

427

Can this be a Material pointer? Which if left empty would just use the first material.

source/blender/makesrna/intern/rna_lanpr.c
65

For consistency with other struct names, there should be no underscore in them.

So it could be LANPRLineLayer, or even NPRLineLayer.

174

enabled -> use

186

Probably not needed to define ranges for colors.

195

Don't use the term "select" for properties and description here unless it's related to selection tools. I think "filter" can work here.

source/blender/makesrna/intern/rna_modifier.c
5941

The standard prefix is use_, not enable_.

5953

Freestyle -> NPR

5985

Don't abbreviate Grease Pencil to GPencil or GP in the user interface.

5996

Multiple -> Multiple Occlusion Levels?

There is no explanation here of what this does, why a user might want to enable this.

6002

The description should not be the same as level_end, and give some explanation of what this does.

source/blender/makesrna/intern/rna_object.c
2326

"Don't use object for LANPR rendering"

"calculate" doesn't explain things at a user level.

source/blender/makesrna/intern/rna_scene.c
7159

Can this be a boolean?

7390–7393

Remove this, renderers should not affect global modifier evaluation like this. It's the user responsibility to do this if needed. Most users are now using Autosmooth rather than Edge Split modifiers anyway, for cases where you just want to modify the normals.

source/blender/modifiers/CMakeLists.txt
42–104

Remove whitespace changes.

source/blender/modifiers/intern/MOD_edgesplit.c
130

Don't add forward declarations in .c files, include the appropriate header.

In general modifiers should not call into editors modules though, so this function probably needs to be in blenkernel.

134

Use bool and true and false.

source/blender/modifiers/intern/MOD_featureline.c
86–87

Why is this a modifier if it does nothing to the mesh?

If it's just a way to store settings on an object, why not add them to object.lanpr?

source/blender/blenloader/intern/readfile.c
6261

Every datablock pointer like this must also be added in library_foreach_ID_link in library_query.c.

Brecht Van Lommel (brecht) requested changes to this revision.Aug 11 2019, 12:06 PM
This revision now requires changes to proceed.Aug 11 2019, 12:06 PM
Antonio Vazquez (antoniov) added inline comments.
source/blender/modifiers/intern/MOD_featureline.c
86–87

@Brecht Van Lommel (brecht) There are usability reasons to define the modifier at Object level. @Matias Mendiola (mendio) and @Daniel Martinez Lara (pepeland) were testing the workflow with different approaches and the best is to keep the modifier here and generate all data for grease pencil.

@Matias Mendiola (mendio) can you give a full explanation of why the modifier must be at object level?

YimingWu (NicksBest) marked an inline comment as done.Aug 11 2019, 1:01 PM

About double precision: LANPR CPU mode requires double precision to work properly. This is discovered the beginning of LANPR's development, single precision floats aren't enough to represent the necessary information needed. Maybe I can try to make some of them who have better tolerance into float instead of double.

release/scripts/startup/bl_ui/properties_lanpr.py
2

This is designed to show multiple feature line modifier configurations at the same place, which is visually similar to particles panel.

source/blender/editors/include/ED_lanpr.h
38

This was copied from my external code. Will remove this.

45–49

Will check them. Thanks.

707

Most of them (except one memory thing) are already BLI/MEM functions. I'll remove the redundant header and implementations.

814

Right... what could be a good place for private ones? Should it be inside editors/lanpr folder? I'll check and modify those needed externally into ED ones.

source/blender/modifiers/intern/MOD_featureline.c
86–87

The design of this modifier is mainly to allow geometry at multiple modifier stages to be extracted for LANPR calculation. Although it's only handling for the moment. If use object.lanpr it's harder to specify "to extract geometry from which stage of the modifier stack".

About double precision: LANPR CPU mode requires double precision to work properly. This is discovered the beginning of LANPR's development, single precision floats aren't enough to represent the necessary information needed. Maybe I can try to make some of them who have better tolerance into float instead of double.

I don't know that it is actually needed. It might, but in my experience it's quite rare that single precision input needs processing in double precision. Changing this may be quite a bit of work though, and I'm not really insisting that we have to do this. But we should not have an entirely separate math library for this reason at least.

release/scripts/startup/bl_ui/properties_lanpr.py
2

Ok, in that case the tab must be in a different order, after the object tab like particles.

source/blender/editors/include/ED_lanpr.h
814

A lot of modules have a file like lanpr_intern.h for this.

source/blender/modifiers/intern/MOD_featureline.c
86–87

Is that really an important feature to have? We have something like that for particles, but has generated more problems than solved them, and we will likely get rid of this.

Renderers work on the meshes as they are output by scene evaluation, hooking into some custom position in the modifier stack can be useful in rare cases but there are also other ways to achieve the same thing with multiple objects. I'm not sure this less common case should influence the overall design like this.

After some modifications, now LANPR completely uses BLI math functions, with several additions of double precision convenience functions.

Double precision is required for at least intermediate results, currently I have converted all the matrix into floats and observed little problem. However if I change intermediate coordinates into float the algorithm will fail to produce correct result in many cases especially when mesh has 2+ levels of subdiv. I'll keep checking which places are mandatory as of using double precision.

The changes are now in temp-lanpr-staging branch.

Sorry I wasn't sure why I clicked abandon? Reclaimed.

This revision now requires changes to proceed.Aug 14 2019, 1:01 PM

After some digging I found out that if I use 32bit float projection matrix, there will still be problems with calculation. This is because LANPR calculates occlusion on projected 2D space, not 3D ray tracing, so precision on the projection needs to be high in order to compensate the z resolution. So I might need to keep the double algorithm for longer time until I found out at which point the precision tolerance is the lowest.

@Brecht Van Lommel (brecht) what do you think about it?

If some computations have to be in double precision then so be it, but all those double math function should be part of our blenlib math library instead of a separate "tmat" library for LANPR.

OK. I'll add my double variants to BLI and keep them minimal. Then I'll provide a separate patch to be reviewed first. Thanks!

YimingWu (NicksBest) marked 18 inline comments as done.Sep 4 2019, 8:12 AM
YimingWu (NicksBest) added inline comments.
source/blender/blenloader/intern/readfile.c
6261

CALLBACK_INVOKE(scene->master_collection->lanpr.target, IDWALK_CB_USER);

I just added this in case ID_SC, seems not crashing. But I don't know if it is correct to do so.

YimingWu (NicksBest) marked 4 inline comments as done.Sep 4 2019, 8:22 AM
YimingWu (NicksBest) added inline comments.
source/blender/draw/CMakeLists.txt
132

I'll remove snake mode completely until a better image filtering solution come up.

YimingWu (NicksBest) marked 25 inline comments as done.Sep 4 2019, 9:11 AM
YimingWu (NicksBest) added inline comments.
source/blender/makesrna/intern/rna_material.c
355–364

Removed for future implementation.

source/blender/makesrna/intern/rna_scene.c
7159

Removed with snake mode code.

7390–7393

Removed this functionality.

YimingWu (NicksBest) marked 7 inline comments as done.Sep 4 2019, 9:15 AM
YimingWu (NicksBest) added inline comments.
source/blender/draw/engines/lanpr/lanpr_all.h
30

Include order fixed.

YimingWu (NicksBest) marked an inline comment as done.EditedSep 4 2019, 4:54 PM

I'm posting the updated patch for Smooth Contour and some GP modifiers as sub tasks here. (Tomorrow) They do not directly interact with LANPR but they will be included anyway. Main part is still being fixing, hopefully be ready before the end of the week.

YimingWu (NicksBest) marked 2 inline comments as done.Sep 5 2019, 5:55 AM
YimingWu (NicksBest) marked 3 inline comments as done.Sep 5 2019, 9:23 AM

Update on the fixes. This is still the LANPR bulk.

There are too many conflicts when updating the modifier code, probably due to the wrong index somewhere in the git. I'll clean up more later.

Dalai Felinto (dfelinto) retitled this revision from soc-2019-npr LANPR Patch to LANPR line rendering (summer of code).Sep 10 2019, 9:56 PM

Changed target material and layer selector from number into string selector.

Multiple layer and material for different line types is implemented.

Auto update in LANPR mode is problematic due to evaluated scene being deleted before background evaluation finishes. Still looking for solution for this.

YimingWu (NicksBest) edited the summary of this revision. (Show Details)

Undone Freestyle changes.

A symbol problem in one of the python script. Fixed. Sorry...

Removed MOD_Featureline.c. No longer needed.

YimingWu (NicksBest) marked 2 inline comments as done.Sep 12 2019, 10:39 AM
Way awarded a token.Sep 13 2019, 8:37 PM