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

Add icons for 3D texture classes #78903

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Jul 1, 2023

image

Oh, and the old Texture3D icon is deleted. Its style clashed with the one I went for here, also it's an abstract class so an icon isn't that important. Texture2D hasn't got one for example

@MewPurPur MewPurPur requested a review from a team as a code owner July 1, 2023 09:14
@AThousandShips AThousandShips added this to the 4.x milestone Jul 1, 2023
@fire
Copy link
Member

fire commented Jul 4, 2023

I can't visually recognize CompressedTexture3D and NoiseTexture3D.

@Zireael07
Copy link
Contributor

Yep, Noise3d looks extremely close to generic Image3d...

@MewPurPur
Copy link
Contributor Author

OK, let me revise

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jul 4, 2023

image

I moved the cube configuration a little so ImageTexture3D has its white in different places. I'm ready to push the changes if they are liked.

@timothyqiu
Copy link
Member

I thought the NoiseTexture3D icon was showing something like fog. But it turned out to be 3D cubes when I open the SVG 🤣

I think using a flat image would be more easier to read. It does not have to be 3D since the image is like a sticker on a box.

@WhalesState
Copy link
Contributor

have a look on this, it may be easier to recognize with a jaggy line
image

@MewPurPur
Copy link
Contributor Author

Squares should be good and familiar from the 2D version. This idea could be used for an icon for the Noise class.

@WhalesState
Copy link
Contributor

Squares should be good and familiar from the 2D version. This idea could be used for an icon for the Noise class.

you can keep the cube and add a jaggy line over it instead of the smaller cubes
image

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jul 17, 2023

I get you, but that's not what I'm saying. I'm saying I want to remain similar to this:

image

Anyway, next take, what do you think? It's not grid-aligned, that's impossible, but hopefully it's distinct enough to make up for it.

image

I personally like old ImageTexture3D more for sure, and honestly I also like the cubes more.

@WhalesState
Copy link
Contributor

WhalesState commented Jul 19, 2023

It looks good and clearer than the previous one, the icons size in godot are 16x16px, you can make it looks sharper if you snap the points to grid if possible, like this small edit. NoiseTexture3D
the issue that makes them not recognizable is that you only have 9x7 pixels inside the cube front face to draw shapes and you should have one pixel margin to not make them connected means 7x5 pixels only.

@MewPurPur
Copy link
Contributor Author

I'm not too strict about the grid, sometimes not making things grid-aligned allows you to make them larger, and therefore clearer. Do you have the SVG of your example? I'm pretty on board with the idea of keeping the 3D-ness, but only as an "easter egg" while the front faces are big enough to look comparable to NoiseTexture2D's squares. I'll test it later though

@WhalesState
Copy link
Contributor

WhalesState commented Jul 20, 2023

Do you have the SVG of your example?

the points are aligned now, it's fine to ignore the grid snapping to pixels but for such small size it will result in blur even if you slightly move a point, and this blur will make it harder to read when they render in the editor.

Texture3D
NoiseTexture3D

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jul 20, 2023

Could we merge this as is, anyway? I'm pretty set on doing a follow-up PR exploring the Noise-related classes, so I could explore more alt designs there, it'd also allow me to maybe change up NoiseTexture2D.

@MewPurPur
Copy link
Contributor Author

#79722 @WhalesState I used the jaggy line here.

@WhalesState
Copy link
Contributor

#79722 @WhalesState I used the jaggy line here.

the jaggy lines looks cool.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 1, 2023

The NoiseTexture3D icon is a bit difficult to make out if you don't know where does it come from.
I think #78903 (comment) might've been better.

@MewPurPur
Copy link
Contributor Author

Alrighty, seems to be what everyone thinks, so I'll do another take on this one!

@MewPurPur MewPurPur force-pushed the forget-dice-we-have-texture-cubes-now branch from 314851f to 414cbe2 Compare August 4, 2023 16:33
@MewPurPur MewPurPur force-pushed the forget-dice-we-have-texture-cubes-now branch from 414cbe2 to 3510b6e Compare August 4, 2023 16:38
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Aug 4, 2023

Haven't changed anything important in this push, I'm still using the initial designs. Can't make anything I like more. And honestly I feel quite restricted on a PR that's about texture cubes, rather than Noise-related icons.

Meanwhile the PR that uses a jaggy line for FastNoiseLite also is getting a bit of a pushback, since the library doesn't have you think about noise functions and a jaggy line is not a familiar association.

So let's pls refine later, I could try to make a PR that re-imagines all three icons in a way that's consistent once this gets in.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, looks good to me. The icons are easily recognizable as 3D, which is usually the most important distinction (as you can't use those in places that expect 2D textures and vice versa).

Screenshot_20230805_012047

Screenshot_20230805_012101

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 5, 2023
@akien-mga akien-mga merged commit 524c8f0 into godotengine:master Aug 7, 2023
@akien-mga
Copy link
Member

Thanks!

@MewPurPur MewPurPur deleted the forget-dice-we-have-texture-cubes-now branch August 7, 2023 16:20
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