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

Example how to repeat texture #11109

Closed
wants to merge 1 commit into from
Closed

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Dec 27, 2023

Texture from ambientCG.

Adding two copies because Bevy ignores loader settings when loading the same file twice.

image

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@matiqo15 matiqo15 added C-Examples An addition or correction to our examples A-Assets Load files from disk to use for things like images, models, and sounds labels Dec 27, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looks like a useful example?

We should avoid using AI generated assets in the repo due to questionable copyright status.

Can you open a bug for not respecting loader settings when loading different files?

@stepancheg
Copy link
Contributor Author

Midjourney says it's fine.

It might be an issue if this asset was distributed with Bevy (to be used in large company), but this does not apply to examples.

Or any suggestions how to make/get a better texture?

@stepancheg
Copy link
Contributor Author

#11111

@alice-i-cecile
Copy link
Member

Midjourney has a vested commercial interest in that position: the law is definitely not settled and we can sidestep the question easily enough.

As for alternate textures, I'd look for a tile pack on itch.io labelled under Creative Commons. I like the looks of this water texture, or perhaps this industrial background.

@stepancheg
Copy link
Contributor Author

Somehow I did not find a texture I liked on itch after searching for 15 minutes.

I picked this one which says it is CC.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Almost every bevy examples puts main() at the top, then setup() then everything else. This example should also follow this pattern.

Most examples also use ..default() instead of ..TypeName::default().

examples/asset/texture_sampler.rs Outdated Show resolved Hide resolved
@stepancheg
Copy link
Contributor Author

(Also I accidentally re-requested review from Alice Cecile).

examples/README.md Outdated Show resolved Hide resolved
@IceSentry
Copy link
Contributor

I still think the comment should explain why it's important for the UVs to be outside of the 0..1 range for the purpose of this example. Beginners don't necessarily know that kind of thing and they might just get confused when they see that code.

My suggested comment was wrong but I didn't mean you should use that one, just something in the same style. Like you said, just repeating what is in the code isn't helpful but explaining why it's like this is important. Especially when it comes to examples.

Maybe something like: "A Quad with UVs outside of the 0..=1 default range, to force the texture to repeat."

Without a comment like this it isn't obvious why you are manually constructing a mesh here.

@stepancheg
Copy link
Contributor Author

why it's important for the UVs to be outside of the 0..1 range for the purpose of this example

  • Because texture is clamped by default, as stated at the top of the file
  • So we are constructing UV range outside of 0..=1, because otherwise what would be the point of this example?

These are quite obvious logical steps if you understand what the problem is. And you probably are, if you looked at screenshot.

There are bazillion things which are not obvious in this example for the beginner, for example:

  • What is UV, and how it is different from XYZ?
  • What is ATTRIBUTE_POSITION?
  • Why we are using OrthographicProjection?
  • Why we are using 2D MaterialMesh2dBundle, but 3D Vec3?
  • ... and so on.

Examples are not meant to be obvious to complete beginners. Examples are meant to require basic understanding of machinery below, so that one thing on top is explained. And the explanation is not mesh generation, it is this line of code:

// Here we override the sampler settings to repeat the image instead.

This is not step by step tutorial from zero knowledge of programming to your shiny AAA game.

If you already know how texture mapping works, but want to know how to make texture repeated, this is the example for you.

Anyway, I changed the comment to:

/// Quad with UV coordinates in range `-1..=2`, which is outside of texture UV range `0..=1`.

A Quad with UVs outside of the 0..=1 default range, to force the texture to repeat

It does not force texture to repeat. Moreover, this quad is used twice: once with default settings, and second time with texture repeated.

@stepancheg
Copy link
Contributor Author

Added another comment on top.

@IceSentry
Copy link
Contributor

I just want a comment to explain why there's a function that creates a quad when bevy already has a quad. I asked that question in the original review because it wasn't obvious on a quick read. It's also why I suggested the order of functions should be main(), setup(), everything else. Putting mesh generation at the top just makes it seem important but since it's not the main point of the example without explanation it just seems like it distracts from the goal.

Again, I know my suggestions are not necessarily the most correct. I just want to show the style of comment I'm looking for. I said the texture repeats because I just couldn't think of a better word. I know it doesn't necessarily repeat. I'm not asking for a comment to explain every basic thing. I just want a comment to explain why this function exists.

Here's another suggestion (again, doesn't have to be used verbatim but in a similar style of not just stating the obvious)

/// Builds a Quad with UVs in the range `-1..=2` to showcase the sampling modes.
/// We need to do this because the built-in Quad always has UVs in the range `0..=1`. 

@stepancheg
Copy link
Contributor Author

stepancheg commented Dec 29, 2023

it wasn't obvious on a quick read
It's also why I suggested the order of functions should be main(), setup(), everything else.
I just want to show the style of comment I'm looking for

Are you sure you are being objective here? Speaking for majority of readers?

Putting mesh generation at the top just makes it seem important

  • I'm not sure such convention exists as far as I'm aware, to put important things first
  • In this example the important part ImageLoaderSettings modification, and the rest is boilerplate, equally unimportant. So there's no code can be put first to make it important (except maybe setup function, but you insisted on putting it after main, contradicting your own recommendations.)
  • Moreover, looking at other examples, it doesn't look like this recommendation is followed.

// ... to showcase the sampling modes

Everything in this PR is to showcase the sampling modes.

I just want a comment to explain why there's a function that creates a quad when bevy already has a quad
/// We need to do this because the built-in Quad always has UVs in the range 0..=1`.

I consider this comment redundant.

  • If we could use Quad, we probably used it
  • We don't have to use Quad even if we could
  • Comments are not meant to explain why something impossible is not implemented, otherwise comments can be infinitely long
  • Explicit coordinates makes code easier to read: it is immediately clear from this example what are texture coordinates

After several iterations of applying your comments, I have a suggestion.

First, we wasted too much time already discussing trivial things.

image

Second, I applied all your suggestions which I believe are beneficial, and even some which, in my subjective opinion, decrease this example quality. Further applying these suggestions would make this example even worse in my subjective view, so I could no longer sign this off.

So can you please take this PR from me, and submit it with your name, in a way you believe it should be done?

@alice-i-cecile
Copy link
Member

Great, I've closed this PR. I'll make an issue to adopt it.

@stepancheg
Copy link
Contributor Author

If @IceSentry wants to do it, fine with me.

If @alice-i-cecile agrees with suggestions by @IceSentry, then maybe I'm wrong.

But just throwing away the PR calling it good-first-never-to-be-resolved issue, does not look constructive.

@alice-i-cecile
Copy link
Member

As an SME-Documentation, I agree with IceSentry's position. Consistency, clarity and polish are critically important for learning materials.

As a maintainer, your behavior here is frustrating and difficult to work with. You were provided with a friendly, constructive review and good faith questions. Rather than fixing the issues or asking questions about why we see it as valuable, you insisted that you knew best, derailed into a condescending explanation of bikeshedding and then said "please take this off my hands". Following the suggestions is not particularly challenging: I suspect this will be adopted pretty readily.

I am genuinely grateful for your contributions: they have largely been high quality, and resolved a number of problems in our code and documentation as you've encountered them. However, this is part of a continuing pattern of hostile and argumentative behavior, particularly when receiving feedback, that is not consistent with our code of conduct. Please pay particular attention to the second and third bullet point listed there.

Differences of opinion are welcome here: it's vital to building something good and steadily improving it. But this is my workplace: vitriol and condescension have no place in it.

github-merge-queue bot pushed a commit that referenced this pull request Jan 1, 2024
Comment incorrect suggests that texture is clamped outside of `0..=1`
range, while it can actually be configured.

CC #11109
github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2024
Encountered it while implementing
#11109.

Co-authored-by: Alice Cecile <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2024
Shortcut to avoid repetition in code like
#11109.

---------

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants