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

Portal occlusion culling [3.4] #46130

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Portal occlusion culling [3.4] #46130

merged 1 commit into from
Jul 14, 2021

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Feb 17, 2021

Adds support for occlusion culling via rooms and portals. This will offer occlusion culling for cameras (including split screen cameras etc) and dynamic lights.
godotengine/godot-proposals#2172

See here for an introduction to portal occlusion culling:
https://www.youtube.com/watch?v=8xgb-ZcZV9s

Builds

See https://docs.godotengine.org/en/latest/community/contributing/testing_pull_requests.html for builds.

Documentation, Tutorial & Demo

In progress at:
https://github.com/lawnjelly/Misc/blob/master/PortalDocs/portals_index.md

About

This is a mostly static system, although portals can be opened / closed at runtime, and moved.

Rooms / portals work particularly well in indoor environments, and in environments that have been sensibly made to take advantage (e.g. canyons). They don't tend to be a good solution for general outdoor environments and open worlds, but can be made use of with some ingenuity.

Implementation

This will probably be drawn out process getting this ready, and I'm aiming for 3.4, as it will require a fair bit of testing and refinement.

There may be design changes that are preferred, so feel free to suggest these, as a lot of this is user interface and user-friendliness, and I'm very much a beginner on the user interface side. I've tried to make it as user friendly as possible but there may be areas that can be improved.

Main functionality

  • Adds 4 new node types: Room, RoomGroup, Portal & RoomManager

simple_rooms

  • Rooms must be run through a conversion phase, either using the editor plugin, or from script, before the portal culling can be active.
  • When the system is active, everything in the 3d view runs through it. Currently I've rigged the gizmos to show up, but the grid does not yet. You can activate and deactivate the system in the editor.
  • At the moment the BVH / octree runs in parallel and is still used for pairing, portals are used for cull_convex only.

Each visual instance has a new portal mode property in the inspector. It can be:

  • Static (in the room system)
  • Dynamic (moves, but cannot change room)
  • Roaming (moves, and can change room)
  • Global (frustum culled only, useful for editor / debug / 1st person gunfire and particle systems etc)
  • Ignore (not shown)

portal_mode

Features

Room bounds (convex hulls)

The convex hull of each room is currently expected to be static. Convex hulls can either be calculated automatically from the geometry within a room or by specifying a manual bound mesh, in both cases qhull or similar is run on the points, and the hull can optionally be simplified.

Rooms must be non-overlapping (except in the special case of interior rooms). A small amount of overlap may be tolerated in practice (probably up to around the player radius).

Portals

Portals (either a MeshInstance with naming prefix Portal_ or a Portal node) should be placed within a source room in the scene tree. The room that the portal links to can either be:

  • Specified manually in the name (e.g. 'Portal_lounge')
  • Assigned in the Portal inspector (this acts by setting the name for you)
  • Auto assigned, if the Portal is named e.g. 'Portal_', or the named destination room is not found. This will attempt to find the closest room next to the portal automatically, using the room convex hulls.

Sprawling

Unlike some portal systems, objects can appear in multiple rooms. This can be important because a large object in a hidden room that spans a portal will otherwise not be shown if the portal is at right angles to the viewer. By registering in both rooms, the object will appear in either case.

Static objects (and lights) during the conversion phase are assigned to a main room (corresponding to their AABB centre), then geometry is followed if they pass through portals, and they are added to each room they reach. Most objects will be in one room, but this allows large objects like floors to register in several rooms, without splitting the object.

In addition, roaming objects can also sprawl between rooms. Sprawling is slightly less accurate for roaming (moving) objects as it only uses the AABB, but is conservative, so an object should always be shown when it should be in view (at the cost of rarely being drawn when it might not be needed to).

To facilitate sprawling, each portal has a tolerance margin, which can either be a global default or custom value. This margin allows walls / doors to poke slightly into neighbouring rooms without being registered. This is useful to prevent slight modelling errors etc, and make everything more user friendly.

I will fill this out more with screengrabs etc.

Videos showing wireframe

https://www.youtube.com/watch?v=8EImtPwh4iM
https://www.youtube.com/watch?v=TvAK_NLlnwQ
https://www.youtube.com/watch?v=Vk8Pgd1kgBM
https://www.youtube.com/watch?v=Wa77LUWnCGc

PVS

In addition to the basic portal culling, the system can optionally build a potentially visible set (PVS) during conversion. For each room in the level, a list of all the other rooms that can possibly seen from this room is calculated, using the portal geometry. This has a number of advantages:

  • Reading a PVS is very fast at runtime
  • It enables some really useful runtime functionality, like the gameplay monitor
    The main disadvantage is that it only works well with a static level layout, even more so than just using portals alone, which rely on a mostly static level but the portals themselves can be moved. The PVS also takes a little while to calculate, but in practice this should not be noticeable except in very complex levels.

The PVS can either be used entirely for culling at runtime, or used in partial mode, which uses the speed of PVS to determine the visible rooms, and combines the accuracy of the portals to get a more accurate cull for individual objects.

Gameplay monitor

When one of the PVS options is selected, you can use a powerful gameplay monitor system, which will provide you with callbacks (as NOTIFICATIONS or signals) when rooms, room groups and roaming objects enter and exit the 'gameplay area'. The gameplay area is not defined by distance from the camera, but by the PVS (potentially visible rooms), or the secondary PVS (PVS plus neighbouring rooms).

This enables some really useful game techniques.

  • You can use this to turn off animation and physics as it works with VisibilityEnabler and VisibilityNotifier nodes automatically
  • You can use this to turn off AI / processing for objects that are not affecting gameplay
  • You can use this to turn on and off directional lights, or effects, such as rain, which you may only want to affect certain RoomGroups (e.g. outside)
  • You can use this to trigger behaviour in the game when you enter / exit rooms / roomgroups, in the same way as you might use an Area3D

Interior Rooms

internal_room
Supports rooms within rooms, or even entire room systems within another, such as a building within one or more terrain rooms. This makes building open worlds much easier, combining outdoor and indoor zones.

At a most basic level, you can have an entire outdoor area as a single room, and each building an interior room / group of rooms (for instance with a few prebuilt building types). The outdoor area will be occlusion culled when inside, and the indoor areas occlusion culled when outside.

Edit

Thanks to lots of help from @qarmin he has helped fix the CI checks with the RIDs in visual server (the intended usage is non-obvious), which should enable getting the system to run smoothly if multithreaded renderer is selected in project settings. 😁 👍
Thanks to @Calinou for making the editor icons.

@Calinou
Copy link
Member

Calinou commented Feb 17, 2021

Ignore (not shown)

What's the difference between the Ignore mode in VisualInstance and disabling the node's visibility using the visible property?

@lawnjelly
Copy link
Member Author

lawnjelly commented Feb 17, 2021

Ignore (not shown)

What's the difference between the Ignore mode in VisualInstance and disabling the node's visibility using the visible property?

Ignore can be made visible but won't ever show. This is used at the moment for bounds. Invisible (and never visible) objects in the system would slow things down. I don't want to delete the optional custom manual bounds, as they need to be reused in subsequent conversions (when making edits). These are used as a guide for the system, but are never intended to be shown (they are indirectly shown however using the editor gizmo). There may end up being a better way of handling this, but it works to start with! 😀

Thinking about it, if it only ends up being used for the bounds, we could eliminate this from the user interface and have it used internally only. I'll keep an eye on whether this seems worth doing.

@lawnjelly lawnjelly force-pushed the portals branch 11 times, most recently from a706745 to c1ab2f3 Compare February 22, 2021 12:43
@HeadClot
Copy link

HeadClot commented Feb 24, 2021

Hey @lawnjelly is there a binary build anywhere for this feature? I am not sure where to look. I really want to try it out with Qodot before it gets merged in to the 3.2-master.

Cannot wait to see this merged into 3.2-master.

@lawnjelly
Copy link
Member Author

Hey @lawnjelly is there a binary build anywhere for this feature? I am not sure where to look. I really want to try it out with Qodot before it gets merged in to the 3.2-master.

Cannot wait to see this merged into 3.2-master.

Sure, for binary builds, you can download any one that passes the CI checks, that has a green tick.

  • Click 'show all checks' above
  • Click e.g. Windows build (or linux) - 'details'
  • Click 'artifacts' in the top right of the browser
  • Select the editor or templates, and it should download.

Just yesterday I've added support for generating primary and secondary PVS from the the rooms / portals, although I haven't pushed it yet, need to decide how to make it accessible. This is primarily intended for the ability to switching on and off physics / AI, although I could also optionally make it able to render directly from the PVS which would be pretty much how quake does it (i.e. there's no runtime limitations on portals then, you can have large numbers of cells for rooms built from a BSP).

Either way, if you wanted to do it automated, you would have to build the BSP / cells / portals yourself though from your brushes. There's probably plenty of example code from quake for this, there may even be some BSP building code in Godot already (I'm pretty sure there is in Godot 2.1). Alternatively you could manually place portals or have a simpler automated system for placing them around the brushes.

core/bitfield_dynamic.cpp Outdated Show resolved Hide resolved
core/bitfield_dynamic.h Outdated Show resolved Hide resolved
editor/plugins/room_manager_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/room_manager_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/room_manager_editor_plugin.cpp Outdated Show resolved Hide resolved
scene/3d/room_manager.h Outdated Show resolved Hide resolved
scene/3d/room_manager.h Outdated Show resolved Hide resolved
scene/3d/room_manager.h Outdated Show resolved Hide resolved
servers/visual/portals/portal_rooms_lookup.cpp Outdated Show resolved Hide resolved
servers/visual/portals/portal_renderer.cpp Outdated Show resolved Hide resolved
@lawnjelly
Copy link
Member Author

lawnjelly commented Feb 27, 2021

@aaronfranke This is still draft - I'll do another pass to clear up before marking as ready for review. Stuff like the braces after if I'm aware of, I tend to not do these for brevity, but convert them for review.

The spare lines or not after braces I'm not clear on whether this is consistent from the existing code, but I'm happy to enforce one or the other. I do agree it is better to just choose one or the other as long as it is consistent. In fact I have an auto tool that does this, but I discovered it created a bunch of diffs in e.g. visual_instance.cpp where this convention had not been used.

Good point about the real_t I will try and convert these.

@huttyblue
Copy link

huttyblue commented Feb 28, 2021

I compiled this PR and gave it a try. It has a lot of issues currently.

Room / Portal linking issues

  • Rooms currently don't handle well if their convex hulls overlap.
    -- Notably when the camera is in a position where two rooms overlap and looking at a portal that connects the two rooms one of them will stop rendering
    -- Commonly seen as the world vanishing for a split second as you cross a portal if the rooms are overlapping slightly at the portal's location

  • Portal mesh bounds are currently added to a rooms convex hull

  • Portals love to unlink from their rooms, often when you instance a scene with a set of portals/rooms, but sometimes they just seem to drop their connections randomly (when in the editor)
    -- Its so unreliable that I've abandoned using the linked-room property in the inspector and instead relied on the portal-name-to-linked-room detection the convert rooms function uses. This becomes a problem when you have 2 portals that link to the same distant room within the same room. As they need to be siblings with identical names.

  • If a circle of portal links is created the user will get "portal depth > 16 errors" and the framerate will tank. Its very easy to accidentally do this.

  • Portals need to be a perfect on-axis quad. The documentation says "The vertices should be positioned on a single plane (although their positioning does not have to be perfect.)" This doesn't appear to be accurate. Portals made of multiple triangles (like filling a cave's edge loop) don't work reliably. And portals rotated at non 90deg angles also seem to randomly not work. (they also make the convex hulls overlap by a tiny amount due to precision loss)

  • Only the first surface (material) of a mesh is considered for the convex hull during room conversion.

In-game runtime issues

  • Meshes set as roaming which aren't children of the rooms do not render unless their transform changes. (basically everything in my game is invisible unless it has an animation player or script moving it)
  • Lights added before the convert-rooms operation don't function
    ( I've worked around this by making a function that removes and re-adds them after the convert-rooms operation runs (which I do in script at the start of my game))
  • Reflection probes have a similar issue to lights
  • Culled meshes still cast directional shadows (or at least ones affected by the above bug do)
  • I need some way to detect if a mesh is culled out so I can turn off its animations, I think my enemy idle animations are eating perf even though they are culled.

Thats all I have for the moment. The PR works but its really picky about those convex hulls and isn't very easy to work with in its current state.

I also did some performance testing to see how big the difference was.
Currently its not too drastic as I suspect the directional shadows and animations still processing in culled areas was reducing the potential performance improvements.

Video of the performance test results.
https://youtu.be/qNSgdCMI_fg

@lawnjelly
Copy link
Member Author

lawnjelly commented Feb 28, 2021

Ah that's great feedback! 😄 Thanks for testing. I'm confident we can work through these, your the first person who has tried afaik (aside from myself) and some of these are issues I had to work through in the modules. It is a major new system and a work in progress, we can refine it to get the best user experience based on feedback.

  • Rooms currently don't handle well if their convex hulls overlap.
    -- Notably when the camera is in a position where two rooms overlap and looking at a portal that connects the two rooms one of them will stop rendering
    -- Commonly seen as the world vanishing for a split second as you cross a portal if the rooms are overlapping slightly at the portal's location

The assumption in rooms / portals is that rooms will not overlap, although for practicality hopefully we can have a bit of leeway. There may be bugs in this situation and workarounds that I can do to effectively solve them (often you have to ignore the first portal, there is already a workaround for this for near clipping planes). This is common issue in rooms / portals.

  • Portal mesh bounds are currently added to a rooms convex hull

I actually added that, because I was thinking about rooms on a terrain, so that you could mark a volume with portals without e.g. building walls and ceiling, or making a manual bound.

Actually thinking about it, for the using portals as part of the bound, it probably makes sense to make it optional because it gives more flexibility, I'll add it as a checkbox in the portal. This way you can use e.g. a square portal even if you have an irregular or rotated shape room.

EDIT: All meshes and portals now have an include_in_bounds boolean, so you can turn this on or off.

  • Portals love to unlink from their rooms, often when you instance a scene with a set of portals/rooms, but sometimes they just seem to drop their connections randomly (when in the editor)

The room / portal graph is only valid from when you convert it currently, once you start moving rooms / portals around in the editor, the graph will be invalid and you will get crazy results. I may be able to rig it up so it reconverts automatically the relevant bits when you move things in the editor, or at least deactivates the portal culling (it does this when you delete a room or portal).

-- Its so unreliable that I've abandoned using the linked-room property in the inspector and instead relied on the portal-name-to-linked-room detection the convert rooms function uses. This becomes a problem when you have 2 portals that link to the same distant room within the same room. As they need to be siblings with identical names.

Ah good point, I haven't fixed this yet. The ability to set the portal links in the editor was an afterthought, the actual linking is done based on the names, and I haven't written code to deal with identical names yet for that bit. And the names not allowed to be identical is a problem. The solution I'd been using so far is detailed in the class reference - you can use an * in the portal name as a wildcard, and anything after is ignored. Thus:
portal_lounge*1, portal_lounge*2 etc all can point into the same room. Otherwise as you say there is a problem. At this stage naming is probably more reliable that using the assign button as you have found.

The naming system is intended to allow you to build the entire rooms / portals and level in e.g. blender, and have it convert in the engine without requiring further editing in Godot. (There is no specific way to tag a room or portal in blender, so I'm resorting to some artistic licence. There may be a better way! But the benefits of allowing full editing in the model package are too much to ignore, particularly when it comes to continuously editing a game level. If you had to re-export from blender, then re-edit rooms and portals in Godot each time it would be a pain to work with.)

  • If a circle of portal links is created the user will get "portal depth > 16 errors" and the framerate will tank. Its very easy to accidentally do this.

At this sounds like a bug where portals are seeing back upon themselves, I'll have a look into this. The limit of 16 is just an arbitrary limit but it sounds like they may be in a recursive loop in this situation (like a snake eating it's tail lol).

  • Portals need to be a perfect on-axis quad. The documentation says "The vertices should be positioned on a single plane (although their positioning does not have to be perfect.)" This doesn't appear to be accurate. Portals made of multiple triangles (like filling a cave's edge loop) don't work reliably. And portals rotated at non 90deg angles also seem to randomly not work. (they also make the convex hulls overlap by a tiny amount due to precision loss)

I'll have a look into this. Ah yes, I may be able to improve the plane finding. Now I think about it I only used the first triangle to determine the plane of the portal, I should average them, that will help with wonky portals. 👍

On the rotated portals : Yes this is expected, the culling is quite naive, it builds culling planes between consecutive portal vertices and the camera source position (which forms a triangle). In a upright door configuration this works well. But tilted, the culling planes may not cull objects (or other portals) as efficiently. There are extra steps possible to increase this accuracy, like adding extra culling planes. This is pretty much free to do when building a PVS but has to be done a little more carefully for runtime portalling, but even so it is pretty cheap.

  • Only the first surface (material) of a mesh is considered for the convex hull during room conversion.

Absolutely correct, my bad (my test level only had one surface). I'll take a look at fixing that.

  • Meshes set as roaming which aren't children of the rooms do not render unless their transform changes. (basically everything in my game is invisible unless it has an animation player or script moving it)

Ah sounds like a simple bug where I don't set them during creation. Will fix this.

  • Lights added before the convert-rooms operation don't function
    ( I've worked around this by making a function that removes and re-adds them after the convert-rooms operation runs (which I do in script at the start of my game))

I'll have a look into this. Probably I'm doing something silly like hiding them.

  • Reflection probes have a similar issue to lights
  • Culled meshes still cast directional shadows (or at least ones affected by the above bug do)

Shadows from directional lights? Directional lights I'm not properly dealing with yet. I had to put in some extra stuff to deal with them in LPortal (my first module). They are generally problematic with rooms / portals because they don't start in a room, and essentially work globally in what they hit, and what they cast shadows on. In fact I think currently they bypass the rooms / portals system.

  • I need some way to detect if a mesh is culled out so I can turn off its animations, I think my enemy idle animations are eating perf even though they are culled.

Yes good point. This is currently also a problem with 3d software skinning (independently of this PR). I could either look at tying in a system to solve that at the same time, or use something similar to godotengine/godot-proposals#2366

Thats all I have for the moment. The PR works but its really picky about those convex hulls and isn't very easy to work with in its current state.

That's still great info. All this stuff is quite fixable, but only becomes obvious with testers! 😁

I also did some performance testing to see how big the difference was.
Currently its not too drastic as I suspect the directional shadows and animations still processing in culled areas was reducing the potential performance improvements.

Yes at this stage I'm concentrating on getting the basic functionality working and usable. In terms of performance, for my modules I found that detaching objects temporarily from the scene tree gave far larger performance increases. On the flip side this gave problems for users who made their render objects and physics objects together - removing the render object removed the physics leading to gameplay issues. The proposal 2366 mentioned above is to look at some of these wider ranging issues, rendering isn't the only bottleneck.

@huttyblue
Copy link

Thanks for the reply. Overall I'm appreciative of this PR, its something Godot really needs.

I actually added that, because I was thinking about rooms on a terrain, so that you could mark a volume with portals without e.g. building walls and ceiling, or making a manual bound.

Thats makes sense. The feature had caught me out when I was making portals a bit larger than they needed as I was still debugging why some of my planes weren't working.

you can use an * in the portal name as a wildcard, and anything after is ignored.

Neat, I wasn't aware of this feature

The naming system is intended to allow you to build the entire rooms / portals and level in e.g. blender, and have it convert in the engine without requiring further editing in Godot. (There is no specific way to tag a room or portal in blender, so I'm resorting to some artistic licence. There may be a better way! But the benefits of allowing full editing in the model package are too much to ignore, particularly when it comes to continuously editing a game level. If you had to re-export from blender, then re-edit rooms and portals in Godot each time it would be a pain to work with.)

Thats mostly what I ended up doing, although I used a blender-python script to make the .tscn file. I was trying to set the linked room there but had to fall back to using names when that wasn't working. (thats also how I was able to make the sibling nodes with the same names)

In terms of performance, for my modules I found that detaching objects temporarily from the scene tree gave far larger performance increases. On the flip side this gave problems for users who made their render objects

Could this be added as a per-node setting near the portal-mode dropdown?

@lawnjelly
Copy link
Member Author

Thats mostly what I ended up doing

That's great because I hadn't really tested the workflow straight from blender, it was all theoretical! 😁

In terms of performance, for my modules I found that detaching objects temporarily from the scene tree gave far larger performance increases. On the flip side this gave problems for users who made their render objects

Could this be added as a per-node setting near the portal-mode dropdown?

Yes potentially. In my earlier module I just had a setting where you could choose whether the culling worked by show / hide, or by detaching from the scene tree. The only snag is, similar to the situation with the PVS, I'd have to use some kind of callback because the relationship from the scene tree (client) to the visual server is designed to be one way for multithreading. But I'm sure it's doable.

@Zireael07
Copy link
Contributor

What do you mean by "detaching objects temporarily from scene tree"? Do the objects run ready() again when reattached/readded?

@lawnjelly
Copy link
Member Author

What do you mean by "detaching objects temporarily from scene tree"? Do the objects run ready() again when reattached/readded?

Well the scene tree is a scene graph with nodes having pointers to children. If you remove nodes / rooms / branches you prevent all processing on that branch. It was a useful approach to take in the module, but some other avenues are available in core. In addition I've made some changes since with the BVH so that invisible nodes no longer take so much CPU, so it may not be necessary. Profiling should reveal what the best course of action is.

I don't think ready() runs again when you reattach, I'm not very familiar with that function. You can do all this from gdscript to test (just remove a child, store it in a reference, and reattach).

@lawnjelly lawnjelly force-pushed the portals branch 2 times, most recently from 3964b98 to 642ebac Compare March 8, 2021 18:38
@Calinou
Copy link
Member

Calinou commented Jul 12, 2021

I'm trying out this PR again with the same demo project as before. It definitely feels more robust than before, but I've still run into usability issues, unfortunately.

Updated testing project: test_portals.zip

This time, I got 2 rooms out of 3 working, but the last one will be culled incorrectly (it feels like it's "inside out" even though the portal autolink works). Also, convex hull generation will create a "prism" shape that's too small to encompass the main room if I move this main room (the one in the middle) to be exactly flush with the other rooms:

Before moving the main room to be flush with other rooms

Before moving

After moving the main room to be flush with other rooms, then converting rooms using RoomManager

After moving

Some more comments:

  • To look consistent with the rest of the editor, the error dialogs that appear when converting rooms should use EditorNode::get_singleton()->show_warning(TTR("...")) instead of OS::get_singleton()->alert().

  • Portal's Linked Room property should only allow selecting nodes of type Room (and perhaps RoomGroup if that's intended). Use the comma-separated PROPERTY_HINT_NODE_PATH_VALID_TYPES for this purpose.

  • RoomManager's Rooms Convert button could be renamed to Convert Rooms.

  • When using Room's Generate Points button, instead of displaying an error message about not replacing existing room points, I'd suggest displaying a confirmation dialog of the sort: "Replace existing room points with generated points?"

    • Or maybe even always overwrite points… As long as the undo/redo system is effective, it's probably fine in practice.
  • The Portal autolink failed message should suggest flipping the portal inside out, since this is one of the most common reasons for autolink failures.

  • The Portal gizmo has different colors on each side which is nice, but I don't know which one is "inside" and "outside" at a glance. It would be useful to display an arrow clearly pointing at a direction (see Adding Raycast3D custom debug shape thickness and color #46529 or DirectionalLight3D for an example).

  • Portal margins no longer have a visible representation with the gizmo, is that intended?

    • Edit: This is because RoomManager's Show Margins is disabled by default. I wonder if it should be enabled by default, as new users may struggle with placing portals accurately.
  • Selected portal gizmos should be translucent so that you can see through them (and unselected portals would be even more translucent). Right now, portal gizmos can get in the way when selected, making designing tunnels more difficult than it should be.

  • What does the "Possible misnamed node" warning mean when using the Generate Points button on a Room? I'm not sure what went wrong exactly here; the message could perhaps be made clearer.

  • Should RoomManager's Camera property be moved to the Debug section in the inspector (and perhaps renamed to Preview Camera)? It should only be set for debugging purposes.

@lawnjelly
Copy link
Member Author

lawnjelly commented Jul 12, 2021

Good observations 👍

  • To look consistent with the rest of the editor, the error dialogs that appear when converting rooms should use EditorNode::get_singleton()->show_warning(TTR("...")) instead of OS::get_singleton()->alert().

Will do.

  • Portal's Linked Room property should only allow selecting nodes of type Room (and perhaps RoomGroup if that's intended). Use the comma-separated PROPERTY_HINT_NODE_PATH_VALID_TYPES for this purpose.

Ah yes this will be better.

  • RoomManager's Rooms Convert button could be renamed to Convert Rooms.

Will do. I think I did have it with that label before, but changed it at some point for consistency with the function name in the RoomManager. I'd also wondered about putting the flip portals button in a menu as it is more rarely used.. but so far there would only be one item in the menu.

  • When using Room's Generate Points button, instead of displaying an error message about not replacing existing room points, I'd suggest displaying a confirmation dialog of the sort: "Replace existing room points with generated points?"

Will do, I didn't know the correct function to call, but now I see it is in EditorNode.

  • Or maybe even always overwrite points… As long as the undo/redo system is effective, it's probably fine in practice.

I'll see whether the undo / redo works. If so that might do the job. I am of course wary of having any options that might overwrite users work, as it can take several minutes to edit a good bound by hand.

I'll try with an arrow. I wasn't sure whether an arrow would be distracting, but we can see how it looks.

  • Portal margins no longer have a visible representation with the gizmo, is that intended?

    • Edit: This is because RoomManager's Show Margins is disabled by default. I wonder if it should be enabled by default, as new users may struggle with placing portals accurately.

Sure it can be enabled by default.

  • Selected portal gizmos should be translucent so that you can see through them (and unselected portals would be even more translucent). Right now, portal gizmos can get in the way when selected, making designing tunnels more difficult than it should be.

They can be made more translucent. I was thinking longterm we could make all the colors editable in the editor settings, but I didn't think it necessary in the first merge just to get some feedback (maybe something you would be interested in, you are a lot better at perfecting the user interfaces).

  • What does the "Possible misnamed node" warning mean when using the Generate Points button on a Room? I'm not sure what went wrong exactly here; the message could perhaps be made clearer.

Ah yes, the generate points functionality is very new (I put it in over last couple days). The reason for the warning is that in order to generate the points, it has to stealthily run a rooms_convert first in order to get sensible planes, and it is the rooms conversion that is generating the warning. It must have found some nodes it didn't like during the convert. Strictly speaking it shouldn't have to convert all the rooms, but it was the easiest way to get it working without introducing further bugs. I can look at making this more efficient once we have some testing.

  • Should RoomManager's Camera property be moved to the Debug section in the inspector (and perhaps renamed to Preview Camera)? It should only be set for debugging purposes.

Yes this is a good idea.

@lawnjelly
Copy link
Member Author

lawnjelly commented Jul 12, 2021

Your test project worked fine, the only problem was you had placed one of the portals in the wrong room. (There were 3 rooms, and the portal was placed in the furthest room from the 2 it was meant to link)

Here's it fixed (I also lightened up the sky a bit as I couldn't see). I'm trying to think whether it will be possible to detect such a situation and make a warning dialog. Ah maybe I could add some kind of warning to the portal gizmo, that's a good compromise. Visual feedback, but not always an error.

Actually that makes me think I can probably produce a warning message if it thinks the portal is the wrong way around. Although that isn't foolproof, so shouldn't be a dialog. I did try this before (autodetecting portal orientation convention) by comparing the normal of the portal to the offset to the room centre. But the problem is that in some outdoor areas (admittedly rarely) you might wish to place rooms on a large terrain that contain no actual geometry, just to be used as logical rooms, and the terrain mesh is shared (sprawled) between them. There is thus in some cases no frame of reference to decide what might be the centre of a room.

calinou_test_portals_second.zip

@lawnjelly lawnjelly force-pushed the portals branch 3 times, most recently from 9b01af0 to 12ae2a9 Compare July 12, 2021 17:13
@lawnjelly
Copy link
Member Author

lawnjelly commented Jul 12, 2021

Here's with an arrow:
portal_colors_blue

I'm spending ages faffing around with the color scheme, but really have no clue what I'm doing - I'm no graphic designer so I'm happy to leave it to someone else to tweak with another PR. 😄

Fixes

  • You now get warnings if it detects possibly placing portals in the wrong room, or facing the wrong way. It also displays icons with the gizmo so you can quickly identify which portals are problematic in a level.

@lawnjelly lawnjelly force-pushed the portals branch 2 times, most recently from 9fd7c5f to 5299599 Compare July 12, 2021 18:43
@Calinou
Copy link
Member

Calinou commented Jul 12, 2021

The new gizmo looks great, thanks 🙂

@Wavesonics
Copy link
Contributor

Wavesonics commented Jul 13, 2021

I took some time to pull down the branch and check it out.

I think maybe this might not work for my use case, but wanted to see if you knew something I didn't.

So my use case is essentially 1 large GridMap which the whole level is laid out in.

image

It appears that the way rooms must work is that the geometry must be segmented under individual room nodes. So there is no way I could make my current setup work w\ this system if I'm understanding this correctly.

Anyway, it's seems to be a pretty nice system over all!, I'm going to cook up a different project to try it out.

@lawnjelly
Copy link
Member Author

lawnjelly commented Jul 13, 2021

Autoplacing portal mode STATIC and DYNAMIC

It appears that the way rooms must work is that the geometry must be segmented under individual room nodes. So there is no way I could make my current setup work w\ this system if I'm understanding this correctly.

I may possibly add this option soon (possibly today). I did have a think a couple of days ago about some use cases where it would be more useful to only put the walls / floors in the rooms, and have the other visual instances (portal_mode static and dynamic) 'auto-placed' into the appropriate room. This is very possible because as long as the room bound is correct, the system can usually identify which room an object should be in and which rooms it should sprawl to (it does this already for moving objects).

There are a few exceptions, where it would be better to place objects in the rooms manually, but this may help people who like to e.g. drag objects around a map in a more freeform way (in the majority of cases).

You could then in theory build the room entirely with points in the room (to form the bound), or just take great care over the geometry for the walls, then place everything else automatically.

I'm just trying to think of a sensible way of exposing this to the user. I don't think trying to autoplace all visual instances in the scene tree would be a good idea, but perhaps all that are in the RoomList branch (but not in a room) could be autoplaced.

This could either be a pre-process, with a button in the editor, or perhaps more usefully do this as part of the room conversion step.

Usability and Design

These are all usability refinements that I'm sure we will make as we go through some betas. There is no way that we will get the design perfect from the first PR (because we don't know ahead of time the problems and use cases) but we can refine it over a few weeks as users test it, and iron out any bugs.

In short, once we have our rooms defined reasonably, and our portals, we are free to modify the design to suit the various compromises:

  • Beginner friendly
  • Powerful
  • Efficiency

In some cases we have to flat out choose between two options, but in many cases we can 'have it all' with some clever design.

@Wavesonics
Copy link
Contributor

Wavesonics commented Jul 13, 2021

The light mapping node does some sort of automatic discovery of nodes and geo that are siblings of it's self, if something like that could work, along with maybe being able to provide a bounding box that we can edit (again in the same way as the light mapper node) that would be pretty killer.

image

@HeadClot
Copy link

Autoplacing portal mode STATIC and DYNAMIC

It appears that the way rooms must work is that the geometry must be segmented under individual room nodes. So there is no way I could make my current setup work w\ this system if I'm understanding this correctly.

I may possibly add this option soon (possibly today). I did have a think a couple of days ago about some use cases where it would be more useful to only put the walls / floors in the rooms, and have the other visual instances (portal_mode static and dynamic) 'auto-placed' into the appropriate room. This is very possible because as long as the room bound is correct, the system can usually identify which room an object should be in and which rooms it should sprawl to (it does this already for moving objects).

There are a few exceptions, where it would be better to place objects in the rooms manually, but this may help people who like to e.g. drag objects around a map in a more freeform way (in the majority of cases).

You could then in theory build the room entirely with points in the room (to form the bound), or just take great care over the geometry for the walls, then place everything else automatically.

I'm just trying to think of a sensible way of exposing this to the user. I don't think trying to autoplace all visual instances in the scene tree would be a good idea, but perhaps all that are in the RoomList branch (but not in a room) could be autoplaced.

This could either be a pre-process, with a button in the editor, or perhaps more usefully do this as part of the room conversion step.

Usability and Design

These are all usability refinements that I'm sure we will make as we go through some betas. There is no way that we will get the design perfect from the first PR (because we don't know ahead of time the problems and use cases) but we can refine it over a few weeks as users test it, and iron out any bugs.

In short, once we have our rooms defined reasonably, and our portals, we are free to modify the design to suit the various compromises:

  • Beginner friendly
  • Powerful
  • Efficiency

In some cases we have to flat out choose between two options, but in many cases we can 'have it all' with some clever design.

Just thought I would reply to this. It would be nice to be able to select the meshes used by the outer shell of the room and be able to click a button in the editor to define the room bounds.

@lawnjelly
Copy link
Member Author

lawnjelly commented Jul 14, 2021

The light mapping node does some sort of automatic discovery of nodes and geo that are siblings of it's self, if something like that could work, along with maybe being able to provide a bounding box that we can edit (again in the same way as the light mapper node) that would be pretty killer.

Being able to edit convex hulls with a gizmo in the editor would obviously be ideal, this is something I have asked about several times on the developer chat. This would be useful not just for rooms, but also for defining hulls for physics. However, I'm not a user interface specialist, and there are no existing gizmos to 'copy from' which do this kind of thing. The AABB editing for say, BakedLight or LightProbes is far simpler and the gizmo support is already there in the codebase with 'handles', but convex hulls are more freeform and I'm not sure we can use the same approach.

But given that Godot is collaborative, I think once we have the basic system merged, then myself or Calinou or a user interface guy can spend some time to work out whether it is possible to make such an editor gizmo.

Just thought I would reply to this. It would be nice to be able to select the meshes used by the outer shell of the room and be able to click a button in the editor to define the room bounds.

It is pretty simple already, but we could very easily have a menu item so when you have a few meshes selected, there is a 'create room' shortcut for this. It would be nice to get some feedback on this though, as a number of Godot features could benefit from such shortcuts, but there's a bit of a balance about not creating confusion for users who aren't using the feature (some people may never use rooms & portals, for example).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much code... Only did a very go-through but it looks good overall. Testing will have to show how it fares in practice.

core/bitfield_dynamic.cpp Outdated Show resolved Hide resolved
core/bitfield_dynamic.h Outdated Show resolved Hide resolved
core/bitfield_dynamic.cpp Outdated Show resolved Hide resolved
core/bitfield_dynamic.h Outdated Show resolved Hide resolved
Comment on lines +446 to +456
if (_flag_warnings) {
if (warning_f) {
WARN_PRINT("QuickHull : !F");
}
if (warning_o_equal_e) {
WARN_PRINT("QuickHull : O == E");
}
if (warning_o) {
WARN_PRINT("QuickHull : O == nullptr");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If doing this we might as well write understandable warnings :) Here they're no longer linked to the actual lines of code where F, O, and E are defined.

Copy link
Member Author

@lawnjelly lawnjelly Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure exactly what these do represent without dissecting the old Quickhull in detail (this might need the original author). I'm hesitant to put in more verbose warnings just in case the wording is incorrect. This is just an attempt to reduce the warnings spam to one of each per hull.. they seem benign.

servers/visual/portals/portal_renderer.cpp Outdated Show resolved Hide resolved
servers/visual/portals/portal_rooms_bsp.cpp Outdated Show resolved Hide resolved
servers/visual/portals/portal_tracer.cpp Outdated Show resolved Hide resolved
servers/visual/portals/portal_types.h Outdated Show resolved Hide resolved
servers/visual_server_callbacks.cpp Outdated Show resolved Hide resolved
Adds support for occlusion culling via rooms and portals.
@akien-mga
Copy link
Member

Thanks! Amazing work :)

@lawnjelly
Copy link
Member Author

Fantastic. I'm hopefully going to add some configuration warnings very soon (once the debate about that function is sorted) and get the docs finalized. 👍

@lawnjelly lawnjelly deleted the portals branch July 14, 2021 13:35
Xrayez added a commit to goostengine/godot-modules that referenced this pull request Jul 14, 2021
This is available in Godot 3.x now, see godotengine/godot#46130.

[ci skip]
@JFonS
Copy link
Contributor

JFonS commented Jul 14, 2021

I'm late to the party (as usual) but I gave it a spin and it works nicely. It's a bit complex to set up, but with good documentation it shouldn't be a problem.

Regarding gizmos: Portals should be easy to do, if all we need is to define a set of points on a plane the current gizmo system should be more than enough, if anyone wants to give it a go I'll be happy to help.

A gizmo for editing convex hulls is a bit more tricky, it would really benefit from all the changes I'm doing to the gizmo system in master, so we can define a subgizmo for each hull face and have the ability to transform them. I do plan to backport those changes to 3.x but with all the recent changes and renames in master it may not be straightforward.

@lawnjelly
Copy link
Member Author

It should be a bit easier now I've figured out how to use the configuration warnings, I'll be doing a PR shortly. 👍

For portals a gizmo editor would be welcome, but luckily I found they were fairly easy to edit just with the data itself - you would normally only be adding an extra point or two, and moving the existing ones slightly to cover an irregular cave entrance or something.

The convex hulls themselves are more tricky to edit with just the data points, but doable - advanced users will appreciate it. It helps if you generate the points with a high simplification selected, so you end up with a reasonable amount of points (<20 or so). So if we can develop a convex hull editor that would be extremely useful, or even being able to select individual points via the editor and having them highlight in the inspector data would help a lot.

The ability to edit the points in the editor was very much a late addition (I didn't realise that it was possible, or would have been so useful), hence it is rather less developed than some of the other features. But I'm sure we can remedy this. 🙂

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.

10 participants