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

Improved view modes #20333

Closed
wants to merge 1 commit into from
Closed

Conversation

nunodonato
Copy link
Contributor

  • Added a clay render mode to the viewport
  • Changed the wireframe mode to unshaded and disabled rendering the environment

This is a followup to #11392, but I cant say it "fixes" it completely :)

Menu:
menu

Clay render mode:
clay

Wireframe without shading and environment:
wire

Changed the wireframe mode to unshaded and disabled rendering the environment
@nunodonato nunodonato requested a review from reduz as a code owner July 21, 2018 18:01
@nunodonato
Copy link
Contributor Author

nunodonato commented Jul 21, 2018

I had a slightly different version of the wireframe mode, without textures, but I guess it doesn't really harm to have those as well.

Also, even though disabling the environment is just 2 clicks away, I thought it would make sense to leave it off in the wireframe mode. In my experience, it gets in the way of reading the lines 90% of the time due to the sky color.

@Calinou
Copy link
Member

Calinou commented Jul 21, 2018

What about naming it "Display Untextured"? "Display Clay Mode" sounds strange to me — also, none of the other display modes have "mode" in their name.

@nunodonato
Copy link
Contributor Author

@Calinou untextured(no texture, but can have different colors) is different than "clay"(same plain material). I don't know what other name could fit better, I'm used to the concept of "clay renders" from 3d software. Let's see if anyone else has other suggestions

@Zephilinox
Copy link
Contributor

Could always just call it Display Clay Textures

@CowThing
Copy link
Contributor

The clay mode uses the default material, but doesn't that have a roughness of 0? Shouldn't the roughness be 1 so it's less shiny and easier to see the lighting?

Also in Viewport.xml the name DEBUG_DRAW_WIREFRAME was duplicated instead of being DEBUG_DRAW_CLAY.

@WiggleWizard
Copy link
Contributor

I would definitely prefer to have the menu item to be called Display Lighting. Although there must be a difference between what you have in mind (clay textures) and showing just lighting?

@mhilbrunner
Copy link
Member

Formatting needs to be fixed for this one :)

@nunodonato
Copy link
Contributor Author

@mhilbrunner I'll fix formatting and whatever else is needed after there is consensus on what to call the "Clay" mode and that this is getting approved. otherwise will be wasting time ;)

@@ -334,6 +334,9 @@
<constant name="DEBUG_DRAW_WIREFRAME" value="3" enum="DebugDraw">
Objects are displayed in wireframe style.
</constant>
<constant name="DEBUG_DRAW_WIREFRAME" value="4" enum="DebugDraw">
Copy link
Member

Choose a reason for hiding this comment

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

Typo here.

@fire
Copy link
Member

fire commented Sep 9, 2018

What is blocking this issue?

@fire
Copy link
Member

fire commented Sep 10, 2018

I had an idea, what do you think about calling it "Display Clay Material" or "Display as Clay" or even "Display Clay"?

@@ -2218,6 +2218,9 @@ void RasterizerSceneGLES3::_add_geometry(RasterizerStorageGLES3::Geometry *p_geo
if (state.debug_draw == VS::VIEWPORT_DEBUG_DRAW_OVERDRAW) {
m_src = default_overdraw_material;
}
else if (state.debug_draw == VS::VIEWPORT_DEBUG_DRAW_CLAY) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -2830,6 +2830,8 @@ void Viewport::_bind_methods() {
BIND_ENUM_CONSTANT(DEBUG_DRAW_UNSHADED);
BIND_ENUM_CONSTANT(DEBUG_DRAW_OVERDRAW);
BIND_ENUM_CONSTANT(DEBUG_DRAW_WIREFRAME);
BIND_ENUM_CONSTANT(DEBUG_DRAW_CLAY);


Copy link
Contributor

Choose a reason for hiding this comment

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

clang-format doesn't seem to like the double blank line here. https://travis-ci.org/godotengine/godot/jobs/406645089#L539

@akien-mga
Copy link
Member

@reduz says he's OK with the feature, but would do the implementation differently and will likely work on it himself (so I'll assign #11392 to him and the 3.2 milestone). As such, closing this PR since it likely won't be merged, but it's linked in #11392 to use as a reference/starting point.

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.

9 participants