Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Lod range #20489

Closed
wants to merge 1 commit into from
Closed

Conversation

Windfisch
Copy link
Contributor

This implements the lod_max_distance / lod_min_distance and hysteresis of GeometryInstance

@akien-mga
Copy link
Member

Could you squash commits together? See PR workflow.

@akien-mga
Copy link
Member

There are also some style issues with operator-aligned assignments, see https://travis-ci.org/godotengine/godot/jobs/408598091 and Style guide.

@Windfisch
Copy link
Contributor Author

Thank you for your feedback! (And sorry for not reading these documents beforehand)
I've fixed the coding style issues and the documentation and have rebased the branch.

@Windfisch
Copy link
Contributor Author

While playing around, I got the following godot crash which I can't reproduce, neither in release nor debug mode: To me it looks unrelated to my patch, but what's your opinion on this?

ERROR: generate: EditorResourcePreviewGenerator::generate needs to be overridden
   At: editor/editor_resource_preview.cpp:55.
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /usr/lib/libc.so.6(+0x368f0) [0x7ff4543d98f0] (??:0)
[2] Reference::reference() (??:0)
[3] Ref<Texture>::ref(Ref<Texture> const&) (??:0)
[4] Ref<Texture>::Ref(Ref<Texture> const&) (??:0)
[5] Button::get_icon() const (??:0)
[6] Viewport::_gui_input_event(Ref<InputEvent>) (??:0)
[7] Viewport::input(Ref<InputEvent> const&) (??:0)
[8] Viewport::_vp_input(Ref<InputEvent> const&) (??:0)
[9] MethodBind1<Ref<InputEvent> const&>::call(Object*, Variant const**, int, Variant::CallError&) (??:0)
[10] Object::call(StringName const&, Variant const**, int, Variant::CallError&) (??:0)
[11] Object::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) (??:0)
[12] SceneTree::call_group_flags(unsigned int, StringName const&, StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) (??:0)
[13] SceneTree::input_event(Ref<InputEvent> const&) (??:0)
[14] InputDefault::_parse_input_event_impl(Ref<InputEvent> const&, bool) (??:0)
[15] InputDefault::parse_input_event(Ref<InputEvent> const&) (??:0)
[16] OS_X11::process_xevents() (??:0)
[17] OS_X11::run() (??:0)
[18] /home/flo/kruschkram/godot/bin/godot.x11.tools.64(main+0xd5) [0x55abf07657ba] (??:0)
[19] /usr/lib/libc.so.6(__libc_start_main+0xeb) [0x7ff4543c606b] (??:0)
[20] /home/flo/kruschkram/godot/bin/godot.x11.tools.64(_start+0x2a) [0x55abf076562a] (??:0)
-- END OF BACKTRACE --

@akien-mga
Copy link
Member

Yeah the backtrace doesn't seem related to your changes. We merged a lot of patches over the last 3 days so there might be some new bugs in the master branch.

@Sslaxx
Copy link

Sslaxx commented Jul 27, 2018

The crash sounds like #20451.

@darrylryan
Copy link
Contributor

This seems to work for me, although the shadow still gets drawn even if the mesh isnt...

@Windfisch
Copy link
Contributor Author

yes, shadows probably need more thinking; should a high-detail-model be selected if the light's camera is close to the object, or if the main viewport's camera (which is the main camera in scenarios with multiple viewports/cameras?)? How to deal with directional lights.

But I guess it would be a good idea if only those meshes draw a shadow that are actually drawn by the camera.

@pgruenbacher pgruenbacher mentioned this pull request Sep 17, 2018
@akien-mga akien-mga requested a review from reduz September 21, 2018 09:20
@TheOnlyJoey
Copy link

I think it is acceptable that shadows are generated based on the high poly mesh to be honest, in most scenario's it creates the most expected result.

@Windfisch
Copy link
Contributor Author

If the purpose of lod_{min,max}_distance is to reduce rendering workload, then this would be counter productive.
Except maybe, if it's really bound to the distance between the object and the light source. However, then the current implementation of the hysteresis needs to be changed to something that actually supports multiple viewers.

@Windfisch Windfisch force-pushed the lod_range branch 2 times, most recently from b8ea182 to 40074ca Compare November 19, 2018 14:30
@Windfisch
Copy link
Contributor Author

I've updated it to support hysteresis with multiple cameras as follows:
The VisualServerScene::Camera struct gets an added OAHashMap<uint32, bool> that keeps track of whether an object has been visible or not in the last frame.
the _prepare_scene() method got an additional parameter of type OAHashMap *. This can be nullptr (for reflection probes, which are not handled by hysteresis) or more likely, just the current camera's map.

This has (almost) no overhead when LOD is not used at all, because all the map lookups are only made for objects that have a LOD range set.

@Windfisch
Copy link
Contributor Author

Unfortunately, objects which are LOD-culled away still seem to cast shadows; why is this?

@akien-mga
Copy link
Member

Discussed this with @reduz on IRC:

16:17 <reduz> As I explained before, I am against adding this now without lod dependency
16:18 <reduz> this needs to be added as 1) lod range 2) lod depndency 3) per material lod ranges
16:18 <reduz> else its not that useful
16:18 <reduz> i was planning to add it after 3.1 is out, since i will rewrite large part of the renderer by then

As such, moving to the 3.2 milestone (might end up as 4.0 depending on whether or not it's bound to the Vulkan implementation).

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 11, 2018
@reduz
Copy link
Member

reduz commented Aug 28, 2019

This is being implemented in a different way in 4.0 branch, so closing this PR. Still thanks for the effort!

@akien-mga akien-mga closed this Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants