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:
Those above will be submitted in other diffs.
| 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 | I'm also guessing that this might be a unintended change too? | |
| 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 | ||
| 7075 | 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 | Yeah, I added one "lanpr" build option in it, and it gets reformatted into this... | |
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.
| source/blender/draw/intern/draw_manager.c | ||
|---|---|---|
| 2867–2871 | 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. | |
| source/blender/draw/intern/draw_manager_texture.c | ||
|---|---|---|
| 106 | Removed this function. Looks like it didn't get used anywhere, | |
| 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 | If this is how clang format wants it to be, then this is fine. | |
| source/blender/makesrna/intern/rna_space.c | ||
|---|---|---|
| 400 | OK Now the "Properties" are back. Have no idea why it get lost... | |
| source/blender/blenloader/intern/versioning_280.c | ||
|---|---|---|
| 3685 | The two changes in this file should be moved down to /* Versioning code until next subversion bump goes here. */ I've ran into multiple files that are not initialized correctly because these only run on too old .blend file versions. | |
Mostly requesting smaller changes, although I found some bugs and have a few concerns.
Checked over changes in:
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:
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. | |
| 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? | |
| source/blender/blenloader/intern/versioning_280.c | ||
| 3685 | 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. | |
| 418–420 | Duplicated definition. | |
| source/blender/draw/DRW_engine.h | ||
| 142 | 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. | |
| 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 :) | |
| source/blender/editors/space_api/spacetypes.c | ||
| 67 | WITH_LANPR | |
| source/blender/editors/space_buttons/buttons_context.c | ||
| 606–614 | Same comment as below. | |
| 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 | ||
| 1003 | 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? | |
| source/blender/makesrna/intern/rna_scene.c | ||
| 2547 | Include ED_lanpr.h instead. | |
| source/blender/python/intern/bpy_app_build_options.c | ||
| 35 | 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. | |
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.
| 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? | |
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!
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)
Changes:
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:
Also, thanks for the fix in handling the file r/w!
@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.
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.