Page MenuHome

LANPR line rendering (summer of code)
Needs ReviewPublic

Authored by YimingWu (NicksBest) on Aug 9 2019, 3:28 AM.
Tokens
"Yellow Medal" token, awarded by franMarz."Love" token, awarded by gaonirico."Love" token, awarded by Okavango."Love" token, awarded by mistajuliax."Love" token, awarded by belich."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
source/blender/editors/lanpr/lanpr_cpu.c
2034

Expand the explanation here. This comment doesn't tell me anything.

2043

dead code?

2260

why caution?

Again add more comments explaining what is going on.

Same with the other caution comments below

2325

Is this still an issue in the code?

If so, then keep the comment of course.

2380

dead code.

2395

dead code?

2513

dead code

2971

Some comments how this works would be nice. Also a comment inside the ifs explaining what (ba[0].r > tba->l && ba[0].l < tba->r) means.

Perhaps not for every if, but just to give people a hint of what it is checking and what it does in these cases.

3503

dead code

3520

Same as the others big functions too. Add some comments explaining how this is supposed to work and add some comments after the if statements.

source/blender/editors/render/render_update.c
101

How long/short term is this solution?

source/blender/makesrna/intern/rna_space.c
400

I don't think these should have been changed? I'm guessing this is a unintended change that got in

source/blender/python/intern/bpy_app_build_options.c
63 ↗(On Diff #19700)

I'm also guessing that this might be a unintended change too?

YimingWu (NicksBest) marked 27 inline comments as done.Wed, Nov 20, 9:12 AM
YimingWu (NicksBest) added inline comments.
source/blender/editors/lanpr/lanpr_cpu.c
1014

I added explanations for the first cases in each categories to improve understanding.

Also fixed one adjacent triangle linking error.

1757

I'm looking through. Will check the rest parts of the code.

1757

This function is obsolete and deleted. Because the object lanpr marks now has its own data structure in struct Object.

2325

Culling is tested ok for the moment, there might be very unlikely that a triangle where we missed to delete due to floating point precision. let's assume we still need this, I added an UNLIKELY marco there.

source/blender/editors/render/render_update.c
101

This is necessary for grease pencil mode to automatically update for each frame. Maybe there will be a better way but this is the most straightforward.

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

Yes it is then defined as a boolean. removed.

source/blender/makesrna/intern/rna_space.c
400

I actually added "Collection" and "LANPR" tabs in it, and it looks like make format did the formatting.

source/blender/python/intern/bpy_app_build_options.c
63 ↗(On Diff #19700)

Yeah, I added one "lanpr" build option in it, and it gets reformatted into this...

YimingWu (NicksBest) marked 3 inline comments as done.

Fixed stuff commented as above.

I found more code style issue including capitalization and formatting problems. I will fix them today.

I'll create temp-lanpr-review branch later and sync with arc then remove the old ones.

YimingWu (NicksBest) marked an inline comment as done.Wed, Nov 20, 9:19 AM
YimingWu (NicksBest) added inline comments.
source/blender/draw/intern/draw_manager.c
2867

Actually it's the corrupted list who caused the manager missed lanpr's free() callback.

I think I'll remove the "solid" draw engine instead of workbench render engine to prevent that from happening.

YimingWu (NicksBest) marked an inline comment as done.Wed, Nov 20, 10:11 AM
YimingWu (NicksBest) marked 2 inline comments as done.Wed, Nov 20, 10:21 AM
YimingWu (NicksBest) added inline comments.
source/blender/draw/intern/draw_manager_texture.c
106

Removed this function. Looks like it didn't get used anywhere,

YimingWu (NicksBest) marked an inline comment as done.Wed, Nov 20, 10:26 AM

Cleaned up, Now use temp-lanpr-review branch.

source/blender/makesrna/intern/rna_space.c
400

It is mostly that you seem to have modifier the strings of other properties too.

For example "Scene Properties" becomes simply "Scene" with your changes.

If seems like you did a search and replace for "Properties" and removed them.

source/blender/python/intern/bpy_app_build_options.c
63 ↗(On Diff #19700)

If this is how clang format wants it to be, then this is fine.

YimingWu (NicksBest) marked 4 inline comments as done.Wed, Nov 20, 1:45 PM
YimingWu (NicksBest) added inline comments.
source/blender/makesrna/intern/rna_space.c
400

OK Now the "Properties" are back. Have no idea why it get lost...

YimingWu (NicksBest) marked 2 inline comments as done.Wed, Nov 20, 1:59 PM

Fixed multisample texture calls due to DRW_ to GPU_ call changes.

source/blender/blenloader/intern/versioning_280.c
3725

The two changes in this file should be moved down to /* Versioning code until next subversion bump goes here. */
OR they should at the very least always be inside the latest (!MAIN_VERSION_ATLEAST( at the end of the file.

I've ran into multiple files that are not initialized correctly because these only run on too old .blend file versions.

Julian Eisel (Severin) requested changes to this revision.Thu, Nov 21, 5:23 PM

Mostly requesting smaller changes, although I found some bugs and have a few concerns.

Checked over changes in:

  • Build-system
  • File read, write & compatibility
  • RNA, DNA
  • Python UI

Mostly a quick pass over everything, e.g. I didn't check every RNA value definition or Python UI definition.


One general concern that I have is that SceneLANPR and CollectionLANPR are non-pointer members. That means their entire size, including size of contained strings is added to each collection/scene - even if LANPR isn't used. A quick check shows that CollectionLANPR has a size of 1.6KB here(!), which will be added to every collection.
We're talking about DNA data here, so every added bit increases .blend file size - every bit is sacred.

Talked this through with @Sebastian Parborg (zeddb), and we decided for the following:

  • Enabling LANPR can allocate the LANPR data and set the pointers.
  • Disabling it again should *not* free the data, so that unintentionally disabling it doesn't cause all setting to be lost.
  • The data should be written to files (if not NULL of course) so it is preserved. We shouldn't purge non-UI data on file reload.
  • Deleting the scene/collection should free the LANPR data.

For the UI part - this adds a collection tab to the Properties, which I'm not sure about. That is a rather significant change that needs broader discussion by the UI team.
I think the new LANPR tab is fine, it only shows up when enabled in the scene settings. Should still be checked on by the UI team though.

release/scripts/startup/bl_ui/properties_collection.py
22–23

These two imports don't seem to be used?

44

All the panel classes miss the COMPAT_ENGINES property definition, which causes errors and prints in the console.
It's not a good sign if an error that by default prints errors to the console in a common case slips through unnoticed. Let's assume this was an exception ;)

release/scripts/startup/bl_ui/properties_lanpr.py
22–23

Unused imports.

30–34

The comment on COMPAT_ENGINES isn't true since poll() is empty. There's no reason to have poll() defined anyway, just remove it.

release/scripts/startup/bl_ui/properties_render.py
677–679

Can be removed.

source/blender/blenloader/intern/readfile.c
6571–6573

Have you tested using this with link/append?
E.g. linking a scene with LANPR data and an otherwise unused material assigned to material_select.

source/blender/blenloader/intern/versioning_280.c
3725

I second this.

source/blender/draw/CMakeLists.txt
183

We usually also add headers to the SRC lists so that IDE project generators include them. lanpr_all.h is missing here.

412–414

Duplicated definition.

source/blender/draw/DRW_engine.h
142 ↗(On Diff #19723)

This and the definiton should be guarded by #ifdef WITH_LANPR.

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

ED_lanpr.h is should be added to util/CMakeLists.txt's SRC

I'd also suggest adding a compile time check for WITH_LANPR here, to ensure this file isn't included (and thus compiled) anywhere without a WITH_LANPR check around it.

You can do it like this:

#ifndef WITH_LANPR
#  error LANPR code included in non-LANPR-enabled build
#endif
38

Having this in the header means the term "real" can't be used for names wherever the header is included. It may also confuse generated documentation.

While probably not a big issue in practice, it's easy to avoid by naming the type real_t instead.
Would generally consider it good practice to avoid naming conflicts with simple, common terms.

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

abs() truncates the double to an int which may actually cause warnings in some compilers (see -Wabsolute-value). Suggest using ABS() instead.

source/blender/editors/render/render_update.c
62

There we go, the entire header is included and compiled even if WITH_LANPR is disabled :)
That's why it's good to check and fail if necessary in the header itself.

source/blender/editors/space_api/spacetypes.c
67

WITH_LANPR

source/blender/editors/space_buttons/buttons_context.c
606–614

Same comment as below.
This switch should never end up calling the lanpr case if WITH_LANPR is disabled. Better guard against that.

source/blender/editors/space_buttons/space_buttons.c
166–168

Would do like this:

case BCONTEXT_LANPR:
#ifdef WITH_LANPR`
  contexts[0] = "lanpr";
#else
  BLI_assert(!"'WITH_LANPR' disabled - should not be possible to access 'BCONTEXT_LANPR'");
#endif
  break;

Other code should ensure the lanpr context is never set when WITH_LANPR is disabled. If this still happens it's a bug that should be caught in some way (e.g. assert).

source/blender/editors/space_view3d/space_view3d.c
1002

WITH_LANPR

source/blender/makesrna/intern/rna_collection.c
379–380

Most properties here don't have a RNA_def_property_update() call. Are you sure this isn't needed?
E.g. when LANPR is displayed in the viewport and you change properties here - don't you have to update what's shown in the viewport?

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

Include ED_lanpr.h instead.

source/blender/python/intern/bpy_app_build_options.c
35 ↗(On Diff #19723)

Having the lanpr item here only causes the bpy.app.build_options.lanpr to be defined, the logic to actually set the value is missing.

You need add the following, see the matching freetype part for example.

#ifdef WITH_LANPR
  SetObjIncref(Py_True);
#else
  SetObjIncref(Py_False);
#endif

Actually, this isn't just a missing initialization, it's worth: It uses the value of some other build option. That is because this is index based, and SetObjIncref() is used to iterate the array.

This revision now requires changes to proceed.Thu, Nov 21, 5:23 PM
YimingWu (NicksBest) marked 2 inline comments as done.Fri, Nov 22, 11:41 AM

Patch currently does not apply

Checking patch source/blender/blenloader/intern/readfile.c...
error: while searching for:
#include "DNA_packedFile_types.h"
#include "DNA_particle_types.h"
#include "DNA_lightprobe_types.h"
#include "DNA_rigidbody_types.h"
#include "DNA_text_types.h"
#include "DNA_view3d_types.h"

error: patch failed: source/blender/blenloader/intern/readfile.c:75

It used to apply at the beginning of this week.

YimingWu (NicksBest) marked 18 inline comments as done.Fri, Nov 22, 12:17 PM
YimingWu (NicksBest) added inline comments.
source/blender/blenloader/intern/readfile.c
6571–6573

Oh I haven't tested that, will try.

This part of the data might be removed due to a change in the LANPR object flags design.

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

Thanks for the suggestion for doing a error like this :)

38

This is fixed with latest code.

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

Thanks! Wasn't aware of that.

source/blender/editors/space_buttons/space_buttons.c
166–168

Undestood. So in this way we still have the code path and can handle situations like this right?

source/blender/makesrna/intern/rna_collection.c
379–380

I'm not quite sure how to properly set the call using prop update callbacks. would something like RNA_def_property_update(prop, NC_SCENE, "rna_Collection_flag_update"); Help?

YimingWu (NicksBest) marked 4 inline comments as done.

Fixed some comments pointed above. I'm looking into the rest.

I checked the data structure of existing LANPR stuff, I think the idea of how to save/load LANPR data is doable. I'll do a following update on this.

About the CollectionLANPR structure, I'm using a string id for target object/material selection. (Think I have found that from somewhere in the existing code), It may needed to be changed to pointers, I need to investigate the libaddr thing when reading file, then the size will be greatly reduced.

Thanks for such a detailed review!

I'm not sure if you understood the save/load comment correctly. I was not referring to the members of the LANPR structures, but how the LANPR structures themselves are added to the scene/collection structures.
E.g. rather than

typedef struct Collection {
...
  CollectionLANPR lanpr;
...
} Collection;

which typically adds 1.6KB to every collection, it should be

typedef struct Collection {
...
  CollectionLANPR *lanpr;
...
} Collection;

which only adds 8Byte.
Of course if LANPR gets enabled for a collection, the 1.6KB would still have to be allocated.

Right. So it's better to use pointers and to only initialize for LANPR configured collection. That might be a better idea. Thank you!

YimingWu (NicksBest) marked 3 inline comments as done.Sun, Nov 24, 3:05 PM

Just checked the runtime cache of LANPR data, the computation intermediate result can't be saved into DNA because it could easily go beyond Gigs of data. Other parts of LANPR data is already in SceneLANPR and CollectionLANPR.

CollectionLANPR is now a pointer to preserve disk space. (File r/w is still a little problematic though)

  • CollectionLANPR changed to pointer.

Changes:

  • Cleaned up thread lock execution path, making LANPR internal state is predictable.
  • Use locked scene loading for LANPR, lock is released when LANPR finishes loading the scene, this avoids accessing potentially deleted scene data from depsgraph when it's running in background thread.
  • Removed camera and scene reference in LANPR_RenderBuffer for the same reason as above.

After this improvement the viewport became very stable, can run for long session without crashing, and no memory leaking is observed.

Two small problems now:

  • Viewport background render doesn't have cancelling. When it's running in background, another call for F12 would result in conflict. Should be easy to fix.
  • Need a safe method to call for redraw when background rendering is completed.

Also, thanks for the fix in handling the file r/w!

Missing a bracket. Fixed.

@YimingWu (NicksBest) Hey - am trying to review the UI here.

First thing is to simply work out how to use it - have you got a doc or something that explains how this is meant to be used? If not, are you on blender.chat so you could explain?

After playing around with it for a while, I was not able to get any lines to show up (which may indicate a UI issue if it's not immediately apparent). Could also be a platform-specific bug or an issue with my build.

Up to date with latest origin/master

Identified cache size problem, where selecting larger GPU cache size may cause intersection lines to disappear. Might be a driver problem. I'm testing with Intel graphics.

use temp-lanpr-review branch to compile.

Warning message for GPU cache size overflow. Useful in big scenes that could hint the user to use bigger sized cache.

Up to date with latest origin/master
Identified cache size problem, where selecting larger GPU cache size may cause intersection lines to disappear. Might be a driver problem. I'm testing with Intel graphics.
use temp-lanpr-review branch to compile.

Currently building LANPR branch for GraphicAll.org, and I am using temp-lanpr-cleanup2 successfully, but get 'not found' if I try the temp-lanpr-review with git. Are any of these changes migrated to 'cleanup2' yet.
Trying to keep the builds contemporary to development progress. Great to see all of the effort. Thank you one and all.

Don't know why but I'm able to push and pull on remote branch temp-lanpr-review. Maybe try git pull once and see? I'm not quite sure about how the script works. You can apply this patch to master and compile. the older branches are not used as of now.