gsoc-2013-bge-lod

gsoc-2013-bge-lod

[Kupoman, 2013-08-01 01:25:51]

I would like to get this work into Trunk for 2.69. Any review would be much
appreciated.

[Moguri, 2013-08-13 21:34:52]

I've done a quick review of the BGE parts.

https://codereview.appspot.com/12216043/diff/1/source/gameengine/Ketsji/KX_Ga...
File source/gameengine/Ketsji/KX_GameObject.cpp (right):

https://codereview.appspot.com/12216043/diff/1/source/gameengine/Ketsji/KX_Ga...
source/gameengine/Ketsji/KX_GameObject.cpp:723: // Handle dupligroups
If we have to do a special case for dupligroups, that must mean that this
function isn't called for every object in the scene. Does mean we need to handle
children as well?

Taking a deeper look, dupligroup members ultimately end up in the KX_Scene's
object list, which is what is iterated for LoD updates, so I'm not sure why this
special case is needed.

https://codereview.appspot.com/12216043/diff/1/source/gameengine/Ketsji/KX_Ga...
File source/gameengine/Ketsji/KX_GameObject.h (right):

https://codereview.appspot.com/12216043/diff/1/source/gameengine/Ketsji/KX_Ga...
source/gameengine/Ketsji/KX_GameObject.h:764: AddLodMesh(
I prefer definitions to be in the source file, not the header file.

[brechtvl, 2013-09-08 14:21:11]

What is the situation now, did any of the game engine developers do a full
review here yet, or the summer of code mentor? If not I'll make some time next
week to do a review here.

Two quick questions:
* Did you test this with amature deform and modifiers on the LOD objects, does
that all work ok?
* I can't see any "hysteresis threshold", to avoid LOD flipping back and forth
when moving near the distance threshold?

[Kupoman, 2013-09-08 21:55:06]

Mitchell Stokes did a review for the BGE parts of the patch. Joerg Mueller
(my mentor) took a quick look over the patch and said it looked fine to him
(response was on the IRC and not on the code review page). Your review
would be greatly appreciated.

I have done some testing with modifiers (I believe the subsurf and decimate
modifiers worked as intended) but I can do some more testing, especially
for armatures.
The hysteresis threshold sounds like a great idea, I will try to get that
implemented on Monday (Pacific Standard Time).


On Sun, Sep 8, 2013 at 7:21 AM, <brechtvanlommel@gmail.com> wrote:

> What is the situation now, did any of the game engine developers do a
> full review here yet, or the summer of code mentor? If not I'll make
> some time next week to do a review here.
>
> Two quick questions:
> * Did you test this with amature deform and modifiers on the LOD
> objects, does that all work ok?
> * I can't see any "hysteresis threshold", to avoid LOD flipping back and
> forth when moving near the distance threshold?
>
>
https://codereview.appspot.**com/12216043/<https://codereview.appspot.com/122...
>

[brechtvl, 2013-09-13 16:57:49]

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ope...
File release/scripts/startup/bl_operators/object.py (right):

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ope...
release/scripts/startup/bl_operators/object.py:783: bl_label = "Setup Level of
Details By Name"
Change to "Levels of Detail", as used by LodGenerate.

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ope...
release/scripts/startup/bl_operators/object.py:892: bpy.ops.object.duplicate()
Should the mesh be linked here instead of duplicate? I'm not sure, but makes
some sense to me to avoid generating too many datablocks.

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ui/...
File release/scripts/startup/bl_ui/properties_object.py (right):

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ui/...
release/scripts/startup/bl_ui/properties_object.py:140: class
OBJECT_PT_levels_of_detail(ObjectButtonsPanel, Panel):
Can this panel be collapsed by default? Should it only show for Blender Game?

Should there be some toggle to disable LoD temporarily without having to remove
the actual LoD data?

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
File source/blender/blenkernel/intern/object.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
source/blender/blenkernel/intern/object.c:939: BKE_object_lod_add(ob);
Is this necesssary, do we really need to create level of detail data on every
object? Things should keep working as expected even if this doesn't exist. Why
even have a LoD level that points to itself?

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
source/blender/blenkernel/intern/object.c:1030: bool BKE_object_lod_check(struct
Object *ob, struct Scene *scene)
Can you rename this to something like BKE_object_lod_can_use? 'check' is kind of
vague.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
source/blender/blenkernel/intern/object.c:1282: static void
copy_object_lod(Object *obn, Object *ob)
I'd use BLI_duplicatelist here instead.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
File source/blender/blenloader/intern/readfile.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
source/blender/blenloader/intern/readfile.c:5038: ob->currentlod =
ob->lodlevels.first;
You need to add code to expand_object to ensure LoD objects get linked in along
with the main object.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
source/blender/blenloader/intern/readfile.c:9557: if (!ob->lodlevels.first) {
Same comment as before here, I think we should not be doing this.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/object/...
File source/blender/editors/object/object_lod.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/object/...
source/blender/editors/object/object_lod.c:63: ot->name = "Add LoD";
No need for abbreviations in the name here.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/object/...
source/blender/editors/object/object_lod.c:89: ot->name = "Remove LoD";
Same here.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/space_v...
File source/blender/editors/space_view3d/drawobject.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/space_v...
source/blender/editors/space_view3d/drawobject.c:3371: if (!ob) ob =
base->object;
Can you simplify this code, so it's just BKE_object_lod_meshob_get(scene, ob),
and it does the active object and NULL pointer check for you?

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/space_v...
source/blender/editors/space_view3d/drawobject.c:3374: dm =
mesh_get_derived_final(scene, ob, scene->customdata_mask);
Why get the derived mesh for the main object only to release it right away, this
should be possible to skip?

https://codereview.appspot.com/12216043/diff/1/source/blender/gpu/intern/gpu_...
File source/blender/gpu/intern/gpu_draw.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/gpu/intern/gpu_...
source/blender/gpu/intern/gpu_draw.c:1395: if (tmp) ob = tmp;
Same comment as for BKE_object_lod_meshob_get, could be a single function call.

https://codereview.appspot.com/12216043/diff/1/source/blender/makesrna/intern...
File source/blender/makesrna/intern/rna_object.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/makesrna/intern...
source/blender/makesrna/intern/rna_object.c:1464: BLI_sortlist(&ob->lodlevels,
lod_cmp);
Could this code be moved in a BKE_object_lod_sort utility function? Prefer to
keep logic like that out of RNA whenever possible.

https://codereview.appspot.com/12216043/diff/1/source/blender/windowmanager/W...
File source/blender/windowmanager/WM_types.h (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/windowmanager/W...
source/blender/windowmanager/WM_types.h:243: #define NC_LOD				(24<<24)
NC seems to be more about datablocks, use NC_OBJECT|ND_LOD instead.

[brechtvl, 2013-09-13 17:05:49]

A big problem with the viewport display of levels of detail is that it's not
working well with the (soon to be multithreaded) dependency graph and dupli
system.

It works now for simple setups, because mesh_get_derived_final will compute the
derivedmesh on the fly. However for correct results with dependencies and
multithreading all derivedmeshes should have been evaluated beforehand in
scene_update_tagged_recursive.

This is a problem because we don't know before drawing which LoD levels will be
needed. I'm not sure what the solution is here..

The other thing is that if you use LoD now, it will still evaluate the full
resolution mesh everytime. I guess if this is some kind of preview for the game
engine that's ok, but if you wanted to use this to speed up editing, it's not
going to help with that.

[brechtvl, 2013-09-14 02:29:11]

The simple way to make it show correct results and work with the depsgraph would
be to add dependencies from an object to all its LoD objects. The problem then
is of course performance, you're evaluating the modifiers for all LoD objects
even if they are not going to be displayed. That defeats the point of using LoD
in the viewport for better performance, though I guess it's mainly intended for
the game engine. If it was implemented that way though you'd probably want an
option to turn off LoD for the entire scene.

I wonder how this works in the game engine. What happens if you use LoD on an
object deformed by an armature or using shape keys? Would it be evaluating that
for all LoD levels even if the object isn't visible? Or is LoD only working for
static objects?

[Kupoman, 2013-09-14 02:33:04]

Fixed most of the issues brought up from code review so far. Thank you Brecht
for the thorough review.

Mostly the levels of detail in the viewport was for preview, with improving
viewport performance as a potential side effect. If this does not really do
anything for the viewport, then I can make the settings a little more BGE
specific.

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ope...
File release/scripts/startup/bl_operators/object.py (right):

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ope...
release/scripts/startup/bl_operators/object.py:783: bl_label = "Setup Level of
Details By Name"
On 2013/09/13 16:57:49, brechtvl wrote:
> Change to "Levels of Detail", as used by LodGenerate.

Done.

https://codereview.appspot.com/12216043/diff/1/release/scripts/startup/bl_ope...
release/scripts/startup/bl_operators/object.py:892: bpy.ops.object.duplicate()
On 2013/09/13 16:57:49, brechtvl wrote:
> Should the mesh be linked here instead of duplicate? I'm not sure, but makes
> some sense to me to avoid generating too many datablocks.

The object is duplicated to allow further editing of the level of detail as well
as the ability to apply the modifier to improve performance. I guess this could
be a linked duplicate, but this would require the user to make the mesh single
user before doing further modifications.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
File source/blender/blenkernel/intern/object.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
source/blender/blenkernel/intern/object.c:939: BKE_object_lod_add(ob);
On 2013/09/13 16:57:49, brechtvl wrote:
> Is this necesssary, do we really need to create level of detail data on every
> object? Things should keep working as expected even if this doesn't exist. Why
> even have a LoD level that points to itself?

Rearranged some code so that only objects with user defined levels of detail
have that base level defined. That level that points back to its object is
useful for consistency and avoiding special cases.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
source/blender/blenkernel/intern/object.c:1030: bool BKE_object_lod_check(struct
Object *ob, struct Scene *scene)
On 2013/09/13 16:57:49, brechtvl wrote:
> Can you rename this to something like BKE_object_lod_can_use? 'check' is kind
of
> vague.

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenkernel/inte...
source/blender/blenkernel/intern/object.c:1282: static void
copy_object_lod(Object *obn, Object *ob)
On 2013/09/13 16:57:49, brechtvl wrote:
> I'd use BLI_duplicatelist here instead.

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
File source/blender/blenloader/intern/readfile.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
source/blender/blenloader/intern/readfile.c:9557: if (!ob->lodlevels.first) {
On 2013/09/13 16:57:49, brechtvl wrote:
> Same comment as before here, I think we should not be doing this.

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/object/...
File source/blender/editors/object/object_lod.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/object/...
source/blender/editors/object/object_lod.c:63: ot->name = "Add LoD";
On 2013/09/13 16:57:49, brechtvl wrote:
> No need for abbreviations in the name here.

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/object/...
source/blender/editors/object/object_lod.c:89: ot->name = "Remove LoD";
On 2013/09/13 16:57:49, brechtvl wrote:
> Same here.

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/space_v...
File source/blender/editors/space_view3d/drawobject.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/space_v...
source/blender/editors/space_view3d/drawobject.c:3371: if (!ob) ob =
base->object;
On 2013/09/13 16:57:49, brechtvl wrote:
> Can you simplify this code, so it's just BKE_object_lod_meshob_get(scene, ob),
> and it does the active object and NULL pointer check for you?

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/editors/space_v...
source/blender/editors/space_view3d/drawobject.c:3374: dm =
mesh_get_derived_final(scene, ob, scene->customdata_mask);
On 2013/09/13 16:57:49, brechtvl wrote:
> Why get the derived mesh for the main object only to release it right away,
this
> should be possible to skip?

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/gpu/intern/gpu_...
File source/blender/gpu/intern/gpu_draw.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/gpu/intern/gpu_...
source/blender/gpu/intern/gpu_draw.c:1395: if (tmp) ob = tmp;
On 2013/09/13 16:57:49, brechtvl wrote:
> Same comment as for BKE_object_lod_meshob_get, could be a single function
call.

Done.

https://codereview.appspot.com/12216043/diff/1/source/blender/makesrna/intern...
File source/blender/makesrna/intern/rna_object.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/makesrna/intern...
source/blender/makesrna/intern/rna_object.c:1464: BLI_sortlist(&ob->lodlevels,
lod_cmp);
On 2013/09/13 16:57:49, brechtvl wrote:
> Could this code be moved in a BKE_object_lod_sort utility function? Prefer to
> keep logic like that out of RNA whenever possible.
Moved most of this to object.c. All that is left is a couple of lines to
actually call BKE_object_lod_sort.

https://codereview.appspot.com/12216043/diff/1/source/blender/windowmanager/W...
File source/blender/windowmanager/WM_types.h (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/windowmanager/W...
source/blender/windowmanager/WM_types.h:243: #define NC_LOD				(24<<24)
On 2013/09/13 16:57:49, brechtvl wrote:
> NC seems to be more about datablocks, use NC_OBJECT|ND_LOD instead.

Done.

https://codereview.appspot.com/12216043/diff/1/source/gameengine/Ketsji/KX_Ga...
File source/gameengine/Ketsji/KX_GameObject.h (right):

https://codereview.appspot.com/12216043/diff/1/source/gameengine/Ketsji/KX_Ga...
source/gameengine/Ketsji/KX_GameObject.h:764: AddLodMesh(
On 2013/08/13 21:34:52, Moguri wrote:
> I prefer definitions to be in the source file, not the header file.

Done.

[Kupoman, 2013-09-14 02:35:49]

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
File source/blender/blenloader/intern/readfile.c (right):

https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
source/blender/blenloader/intern/readfile.c:5038: ob->currentlod =
ob->lodlevels.first;
On 2013/09/13 16:57:49, brechtvl wrote:
> You need to add code to expand_object to ensure LoD objects get linked in
along
> with the main object.

Is there some documentation to help me understand this part of the code better?
Also what should currently be breaking with things as they are so I can be sure
to test if I have done things properly? I have been able to save and load files
with level of detail systems as well as link them into other blend files.

[brechtvl, 2013-09-16 12:20:12]

On 2013/09/14 02:35:49, Kupoman wrote:
>
https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
> File source/blender/blenloader/intern/readfile.c (right):
> 
>
https://codereview.appspot.com/12216043/diff/1/source/blender/blenloader/inte...
> source/blender/blenloader/intern/readfile.c:5038: ob->currentlod =
> ob->lodlevels.first;
> On 2013/09/13 16:57:49, brechtvl wrote:
> > You need to add code to expand_object to ensure LoD objects get linked in
> along
> > with the main object.
> 
> Is there some documentation to help me understand this part of the code
better?
> Also what should currently be breaking with things as they are so I can be
sure
> to test if I have done things properly? I have been able to save and load
files
> with level of detail systems as well as link them into other blend files.

When I link an object with LoD levels from another file, its LoD level objects
are not linked in along with it now, the object pointers are cleared.

expand_object should call expand_doit on all other datablocks that it wants to
be linked in automatically along with itself.

[brechtvl, 2013-12-15 15:13:49]

What is the status here? This seems close to ready for 2.70, just some minor
things to fix?

[Kupoman, 2013-12-16 08:44:01]

On 2013/12/15 15:13:49, brechtvl wrote:
> What is the status here? This seems close to ready for 2.70, just some minor
> things to fix?

As far as I know most (if not all) of the issues were addressed except for
potential conflicts with depsgraph refactors. I have created a Differential
Revision to continue this process on the new tools:
http://developer.blender.org/D109

Hopefully I did it correctly.