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

Improves Books by Standardizing "islmDniBook_page/side" #243

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

patmauro
Copy link
Contributor

@patmauro patmauro commented Sep 28, 2023

Equivalent to MOULa content/138. Improves appearance of books in base game by ensuring all references to "islmDniBook_page" and "islmDniBook_side" are consistent in all PRPs. This is a 1-1 texture improvement for which there is already precedent in-game and does not introduce anything new, rather just updates all PRPs for consistency.

The equivalent content/138 is included in MOULa Q32023 and has been thus far well-received, but I will walk back the changes on OU if there are significant concerns over accepting this into H'uru, as I would strongly prefer these stay consistent and not diverge. For this PR, the changes have been recreated from scratch for H'uru, building on top of H'uru's most recent PRPs (and making the spine and page changes separate commits). All affected ages have been tested in the H'uru client where possible, and no apparent LOC issues have been identified.

I stand by the approach here: it looks good, behaves well, and has minimal impact.

image

As there were previously some initial concerns over these sizes seeming unusual for these textures, as well as some hesitation over the precedent an update like this might set, let me be very clear about what this update DOES and DOES NOT do:

What this update DOES do:

  • all instances of islmdnibook_side are updated to be consistent with the size already used in ahnonayCathedral, garrison, gira, greatTreePub and nexus.
  • islmDniBook_page variants are updated to be consistent with the size already used by ahnonaycathedral, city, gira, greatTreePub, negilahn, payiferen and tetsonot & the smudgeless variant (similar to xbooknewpage) already in use in ahnonaycathedral & greatTreePub.

What this update DOES NOT do:

  • Introduce any NEW assets
  • Introduce any texture sizes without existing precedent (in other words - nothing is being increased in size just for the sake of adding a new, sharper texture)
  • Modify any textures OTHER than these three files and their variants. (ie - Ahnonay uses an unusual custom book page texture that is also an inconsistent size, but this is not in scope here and has not been modified.)
  • Account for duplicate textures that may exist due to the addition of additional content over time. (However, I have made a note of these cases)

Other Important Notes:

  • Spine texture filename is always of the format *0#0 (or blank) and does not correlate in any meaningful way with size, which is seemingly random. These are thus the most inconsistent and most in need of a refresh here.
  • On the other hand, page texture filenames do use specific language: *0#0 indicates original size, *0#2 indicates "medium" size, *0#3 indicates smallest size was used.
  • "Page" and "Page1" are considered DIFFERENT textures - you will note there are TWO source textures for this provided from Intangibles, even though these images are identical. The reason for this is at lower resolutions, these "become" different, distinct textures - "page1" is smoother, while the other, "page" is more detailed & grainy - essentially, different compression methods. But these "collapse" down into the same texture at standard resolution. Being intentional, this should be left alone and not considered duplicate textures. If, hypothetically, someone wanted to create variants of these textures that replaced all instances of "page" with a sligtly darker & granier variant to make this more accurate to the lower-res texture, they are welcome to do so, but it is not in the scope of what I was looking to accomplish here - nor is it consistent with how "newer" books that simply use two instances of xbooknewpage look.
  • city, gira, negilahn, neighborhood, payiferen and tetsonot do contain fully redundant textures that can be collapsed down into one shared asset. Addressing this was not in the scope of this update.

Hypothetical next steps for a followup project:

  • Hunt down instances like in ahnonay where an alternate page texture is used that is not consistent with the usual stamps, etc. There may be some overlap with the ongoing effort to standardize stamps.
  • For consistency, all page & page1s COULD be re-named islmdnibook_page*0#0.hsm & islmdnibook_page1*0#0.hsm (or islmDniBook_page.dds & islmDniBook_page1.dds) - however, leaving these alone may also be useful, as indications of the original sizes may be helpful reference.
  • the duplicates in city, gira, negilahn, neighborhood, payiferen and tetsonot should be collapsed down into one shared asset. However, this would only save about 1mb, so YMMV.
  • To significantly save space, you could merge down ALL instances of islmdnibook_page, islmdnibook_page1 and xbooknewpage into a single shared asset per PRP.

See attached txt for the full list of PRPs & files this applies to.
Affected_textures.txt

@Hoikas Hoikas added visual An issue or fix that is primarily visual subjective An issue or fix that is artistic or creative in nature labels Oct 13, 2023
@patmauro
Copy link
Contributor Author

patmauro commented Nov 14, 2023

This has now been merged into MOULa; are there any objections to inclusion in this repo? While I understand there were some initial concerns, I believe I have provided sufficient explanation & justification above for why this is the appropriate approach for this enhancement.

@Hoikas
Copy link
Member

Hoikas commented Nov 16, 2023

This is on my to-review list. Content PRs are always the worst to review, and this one touches lots of PRPs, so it's very much been on the back-burner. Hopefully I (or someone else) can review the PRC diff before the end of the holiday season.

@patmauro
Copy link
Contributor Author

patmauro commented Apr 2, 2024

I see this currently has a conflict due to a recent change to Kveer_District_Textures.prp that was recently merged as part of #253. I will grab this new version of Kveer_District_Textures and attempt to re-apply the fix in an effort to resolve, but I'd also respectfully request that this PR be given a formal review as soon as possible (once this is resolved) in order to avoid any additional conflicts in the future. Again, note that the equivalent PR has already been accepted in MOULa, and this submission does not violate the content guidelines established for moul-assets.

@patmauro
Copy link
Contributor Author

patmauro commented Nov 2, 2024

Circling back to this one - I get this is a really hard one to evaluate since it touches so many PRPs. I'd be open to radically re-interpreting this one; perhaps breaking it down into a series of much smaller pull requests that are easier to handle. That said - if we can look at the root issue holistically, perhaps we can settle on a specific "if size = x, replace with y" strategy we could then extrapolate to all other examples.

In short: the key thing I was trying to address here is (1) many pages textures have ugly yellow splotches left over from an older version of the texture, and (b) many of the book "side" textures are hideously ugly and below a standard that should have been acceptable even back in 2003, let alone 2023. My solution was to make sure every version of this texture in the game was always the same size across the board (which means the biggest one present wins out), but I'm open to a more flexible solution if there are concerns. The main thing is, whatever we agree on, I will want to sync this back into OU, because I really want to prevent these PRPs from diverging any more than they have to.

@Hoikas
Copy link
Member

Hoikas commented Nov 3, 2024

I get this is a really hard one to evaluate since it touches so many PRPs.

I think that's the big hold up here. PRP changes are difficult to review at best, and this big delta is just not something that I've really had a chance to examine from either a visual or technical perspective. Let me find some time for that and get back with you. I'm not sure we need or want to bike shed every single change, but we may want to discuss some things here.

@patmauro
Copy link
Contributor Author

patmauro commented Nov 4, 2024

I get this is a really hard one to evaluate since it touches so many PRPs.

I think that's the big hold up here. PRP changes are difficult to review at best, and this big delta is just not something that I've really had a chance to examine from either a visual or technical perspective. Let me find some time for that and get back with you. I'm not sure we need or want to bike shed every single change, but we may want to discuss some things here.

Totally understood, yeah. Let me go ahead and move this one to a Draft PR instead, and I'll think about different strategies to approach this as well.

@patmauro patmauro marked this pull request as draft November 4, 2024 00:43
@patmauro patmauro mentioned this pull request Nov 5, 2024
@Hoikas Hoikas added the conflict This pull request modifies binary files in a way that conflicts with history or other pull requests label Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict This pull request modifies binary files in a way that conflicts with history or other pull requests subjective An issue or fix that is artistic or creative in nature visual An issue or fix that is primarily visual
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants