Page MenuHome

soc-2019-npr LANPR Patch
Needs RevisionPublic

Authored by YimingWu (NicksBest) on Fri, Aug 9, 3:28 AM.

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.

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

what are the minimum specifications for using LANPR?
I tried a branch LANPR on windows 10 and both the radeon hd 7670 and the intel hd 4000, entering the LANPR engine, show me all black or transparent(both gpus work well with eevee)

sorry if i post this comment here

what are the minimum specifications for using LANPR?
I tried a branch LANPR on windows 10 and both the radeon hd 7670 and the intel hd 4000, entering the LANPR engine, show me all black or transparent(both gpus work well with eevee)
sorry if i post this comment here

You may need to add at least one line layer and click update. LANPR consumption is small for small scenes. Document draft is on the wiki: https://wiki.blender.org/wiki/User:Yiming/GSoC2019/LANPR_Document

You may need to add at least one line layer and click update. LANPR consumption is small for small scenes. Document draft is on the wiki: https://wiki.blender.org/wiki/User:Yiming/GSoC2019/LANPR_Document

I don't want to disturb you here ... here is a link on devtalk that describes what happens ..

https://devtalk.blender.org/t/gsoc-2019-lanpr-development-and-feedbacks/7716/33?u=nokipaike

This patch contains only LANPR and other necessary modifications in order to run LANPR itself correctly.

Other touched files are for:

  • Freestyle edge mark to LANPR compatibility.
  • Feature line modifier (which is now part of LANPR to select object)
  • GPencil flag defines dedicated to LANPR.

Compiles and runs correctly.

YimingWu (NicksBest) edited the summary of this revision. (Show Details)Sun, Aug 11, 10:18 AM

Fix new line at the end of the files.

I have looked at the style of your code and I have seen that you are using a lot of variables for flags, but you could join all of them in one flag variable and use a bitflag enum.

Also, some variables and properties need a renaming.

source/blender/makesdna/DNA_material_types.h
190

Do you need so many variables? why not a bitflag enum?

source/blender/makesdna/DNA_meshdata_types.h
372

I don't know FreeStyle code but... are you sure to rename this structs? are you breaking old code? why don't add your own structs for LANPR?

source/blender/makesdna/DNA_scene_types.h
1653

Maybe too many variables to set flags. Look my comment below about using an enum.

You can see a lot of examples in Blender with variables named "flag" for using a bitflag enum.

1699

Better naming "use_***"

Do you really need 3 variables... why don't use only one field and a bitflag enum?

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

Not totally sure, but iirc the style rules advice to name these properties as "use_xxxxx" not "enable_xxx"

More properties with same naming below.

425

In tooltips use full name "Grease Pencil"

436

Maybe better naming would be "level_start"

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

Better "use_normal"

93

Maybe "Start" is more standard that "begin".... normally the naming is Start/End

137

What is QI? maybe we need a better naming/tooltip here

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

Better naming "level_start"

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

Better naming "use_****"

7369

Why not bitflag enum for all these flags?

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

Why you define here, not in header file?

source/blender/modifiers/intern/MOD_featureline.c
77

Do you need this method?

99

Add { } to single if lines or do in a simple

return (!flmd->target);

YimingWu (NicksBest) marked 6 inline comments as done.Sun, Aug 11, 11:39 AM
YimingWu (NicksBest) added inline comments.
source/blender/modifiers/intern/MOD_edgesplit.c
130

This function is from editor/lanpr, and I can't seem to find an appropriate header to put this in, should not be in BKE_ of course but shall I include ED_lanpr.h also?

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

Not sure. @Brecht Van Lommel (brecht) What do you think, what is the best place to put this code or it's ok to keep the definition here?

Here are some quick comments from reading through part of the code, not a proper review yet.

There is still a lot of stuff here, which makes it hard to review. There's a few things I see that could be submitted as separate code review, there may be more.

  • Collection properties tab
  • Freestyle edge/face mark changes.
  • Math utility functions added to blenlib

This renames a lot of things from freestyle to LANPR. Does freestyle still work correctly with this patch? If all this is doing is renaming some things from Freestyle to LANPR, then what I suggest to do instead is to rename those things to generic "NPR" (e.g. CD_NPR_EDGE) and submit that as its own code review.

LANPR should not come with its own math library, listbase and other utility functions. The existing functions in blenlib should be used, or new ones should be added there as needed. A lot of the math code is double precision, is there a specific reason this is needed for LANPR more than other Blender code? All the other Blender and GLSL code is single precision, it's not immediately obvious that using it just for LANPR is particularly helpful.

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
733

Don not leave in commented out code anywhere, unless there is also a comment explaining why it needs to be there.

1096

Options -> Settings

source/blender/draw/CMakeLists.txt
132

"Deprecated" usually means that something is going to be removed in the future, but can't be done immediately because of reasons like backwards compatibility.

What the is the plan for this snake mode? In general it's better to have fewer rendering modes, since it's easier for users if there is a single mode that has all the features rather than multiple with their own limitations.

source/blender/draw/engines/lanpr/lanpr_all.h
30

We have guidelines to group headers by module and sort them alphabetically, see how other files do it.

63–64

We already have functions for this, don't duplicate them.

66–67

There should already be an existing math function for this.

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

This should not be used, especially not in a public header that affects other code.

45–49

We have BLI_sys_types for things like this.

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/editors/lanpr/lanpr_chain.c
2

All files should have license headers.

source/blender/editors/lanpr/lanpr_cpu.c
2764

The long should not be used, it has a different size on different 64 bit platforms.

source/blender/makesdna/DNA_scene_types.h
398–400

Remove whitespace changes.

source/blender/makesrna/intern/CMakeLists.txt
28–95

Remove whitespace changes.

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"?

373–375

Use enum rather than hardcoded 0/1/2.

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_material.c
355–364

You can use RNA_def_property_boolean_negative_sdna rather than writing custom functions for this.

But even better would be to just have matching variables in DNA and RNA.

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
2317

INHERENT -> INHERIT

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–103

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.Sun, Aug 11, 12:06 PM
This revision now requires changes to proceed.Sun, Aug 11, 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.Sun, Aug 11, 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.Wed, Aug 14, 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!