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

Expose World3D.compositor #92383

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nonchip
Copy link

@nonchip nonchip commented May 26, 2024

this closes godotengine/godot-proposals#9815

I'm positive this just got missed in the Compositor implementation, since i could find no mention of its omission or any reasons for it (and it only makes using World3D resources more cumbersome).

@nonchip nonchip requested a review from a team as a code owner May 26, 2024 11:09
@nonchip nonchip changed the title exposed World3D.compositor Exposed World3D.compositor May 26, 2024
@AThousandShips AThousandShips changed the title Exposed World3D.compositor Expose World3D.compositor May 26, 2024
@AThousandShips AThousandShips requested a review from a team May 26, 2024 11:14
@AThousandShips AThousandShips added this to the 4.x milestone May 26, 2024
@AThousandShips
Copy link
Member

AThousandShips commented May 26, 2024

This will be overridden by the one in the first WorldEnvironment currently:

void WorldEnvironment::_update_current_compositor() {
WorldEnvironment *first = Object::cast_to<WorldEnvironment>(get_tree()->get_first_node_in_group("_world_compositor_" + itos(get_viewport()->find_world_3d()->get_scenario().get_id())));
if (first) {
get_viewport()->find_world_3d()->set_compositor(first->compositor);
} else {
get_viewport()->find_world_3d()->set_compositor(Ref<Compositor>());
}
get_tree()->call_group_flags(SceneTree::GROUP_CALL_DEFERRED, "_world_compositor_" + itos(get_viewport()->find_world_3d()->get_scenario().get_id()), "update_configuration_warnings");
}

So it doesn't seem to be overlooked IMO

@nonchip
Copy link
Author

nonchip commented May 26, 2024

@AThousandShips that is intended+documented, just like the environment and camera_attributes.

what was overlooked was exposing it in the actual resource where it is provided to the Viewport, thus forcing us to use the builtin WorldEnvironment node to affect that one specific property only.

@AThousandShips
Copy link
Member

AThousandShips commented May 26, 2024

The documentation should be updated on the World3D then I'd say to clarify this

@nonchip
Copy link
Author

nonchip commented May 26, 2024

@AThousandShips i'm not sure i understand what you mean, i took the documentation of the other properties that work exactly the same way and replaced the names. how should this be documented differently? should maybe all of them be documented a bit more thoroughly? maybe a note in the class doc in general about how WorldEnvironment regularly writes to the World3D?

@AThousandShips
Copy link
Member

(You don't need to tag me I'm right here, just adds annoying pings)

It should match the part with "The default ... if not set on Camera3D." of camera_attributes

The rest is out of scope for this but this is a very important thing to note, or people will be confused when the compositor they assigned is suddenly replaced when a camera or WorldEnvironment enters the scene

@nonchip
Copy link
Author

nonchip commented May 26, 2024

the issue there is that the Camera3D logic does actually preserve the default value while tracking the temporary override by "enabled" cameras, while WorldEnvironment just writes all over all of those properties directly. I think I'll add a "this will be overwritten by WorldEnvironment nodes in the world", the "default" implies it stays the default.

@AThousandShips
Copy link
Member

AThousandShips commented May 26, 2024

I'd say it doesn't imply that and that it should be uniform, so I'd say the documentation should be the same just with Compositor instead

Note that the proposal that proposed this feature only mentioned WorldEnvironment and Camera nodes, not World3D, so I think this was by design:

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
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.

Expose World3D.compositor
2 participants