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

Texture scaling during export, v2 #47

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

Conversation

Venomalia
Copy link
Contributor

@Venomalia Venomalia commented Feb 16, 2022

improved and expandable variant of #46

External scaling is currently not implemented, but can be easily added if required.

grafikgrafik

@iwubcode
Copy link
Owner

iwubcode commented Feb 16, 2022

Nice work!

I don't understand the need for the "Dynamic Size". I imagine it will be confusing to users just seeing the UI. A simple 2x, or 4x scale should be good enough. The real importance is the size (and quality) of the host key texture because Dolphin just does a simple nearest neighbor copy of the file to the sub region.

Some other thoughts:

  • Even if we keep the "Dynamic Size", the UI to change the drop down to be something else seems odd to me
  • I'd remove 1x from the factor drop down. If "None" is specified, I'd disable the drop down instead of hiding it

I'm fine waiting on the "Extern" functionality, though we already have games that it'd be useful to use AI upscaling for.

@Venomalia
Copy link
Contributor Author

Venomalia commented Feb 17, 2022

Dynamic had more the idea to make sure that the regions have a certain size. if you select 64, the factor where all regions are at least 64 pixels large is chosen.
however, it is not necessary, there are other functions that I find more important.

I'd remove 1x from the factor drop down.

with 1x you could scale back to the original size, since it does not mean any effort i would keep it.

If "None" is specified, I'd disable the drop down instead of hiding it.

seems reasonable to me. 👍

externally I see a reason to support ESRGAN and xBRZ.
however, we could also now enable the replacement of a single texture via a context menu or a reload function, because this would now be detected and the regions will be automatically adjusted.

@iwubcode
Copy link
Owner

iwubcode commented Feb 17, 2022

Dynamic had more the idea to make sure that the regions have a certain size. if you select 64, the factor where all regions are at least 64 pixels large is chosen.
however, it is not necessary, there are other functions that I find more important.

I see, it's definitely an interesting idea. Would you mind if we wait and see if users are fine with the simple scale factor approach? Not that dynamic is bad, just more complexity that may not be needed.

with 1x you could scale back to the original size, since it does not mean any effort i would keep it.

Ah, I was still thinking this was on export only. So my question is, do we need to modify the texture size during editing? If they can zoom, I see little reason to bother modifying the size during runtime. Just seems like more book keeping. What do you think?

externally I see a reason to support ESRGAN and xBRZ.

Yeah, it's not critical at this point. I like what you have, let's try this an then worry about external later if we need it.

however, we could also now enable the replacement of a single texture via a context menu or a reload function, because this would now be detected and the regions will be automatically adjusted.

Oh yeah, targeting individual textures would be interesting. Nice idea for a future enhancement!

@Venomalia Venomalia force-pushed the AutoscalingV2 branch 2 times, most recently from 452a9ab to e08dcc5 Compare February 17, 2022 10:53
@Venomalia
Copy link
Contributor Author

I have updated it now.

Ah, I was still thinking this was on export only.

yes it is only when exporting, however you can import and export to the same or a different location.

/// <summary>
/// Number of pixels that the smallest region should have at least after exporting, when the Dynamic mode is used.
/// </summary>
public int SelectedRegioneSize { get; set; } = 96;
Copy link
Owner

Choose a reason for hiding this comment

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

This can be removed as it isn't used anymore.

@iwubcode
Copy link
Owner

iwubcode commented Feb 18, 2022

I'm sorry for nitpicking so much on this review!

I see what the "Change Size" is for but I think it won't be obvious to the user what to choose (how do they remember what values their textures were saved with months later?) and in general the name isn't clear (at least I could see that). Therefore I have a suggestion:

  • Remove "Change Size"
  • Save off the "Factor" globally (much like the "Generated file name", gameid, etc)
  • When loading the UI still default to 4 if there is no saved value but if there is a factor use that instead. (ex: I export with a factor of 2 then later import that file, I should see that displayed)
  • Later, if we ever add the ability to scale individually, you can use the ImageWidthScaling to set the factor for that particular image

@Venomalia
Copy link
Contributor Author

have made all changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants