Skip to content

fix(extended translucency): apply extra data and implicit alpha#1365

Merged
alandtse merged 3 commits into
community-shaders:devfrom
ArcEarth:extended_translucency_fix
Aug 8, 2025
Merged

fix(extended translucency): apply extra data and implicit alpha#1365
alandtse merged 3 commits into
community-shaders:devfrom
ArcEarth:extended_translucency_fix

Conversation

@ArcEarth
Copy link
Copy Markdown
Contributor

@ArcEarth ArcEarth commented Aug 4, 2025

  • nif setting should ignore SkinnedMeshOnly
  • meshes with lightingShaderProperty.alpha < 1 have implicit alpha and should not be ignored for easy testing
    Also clarified the cpu code for translating nif setting into descriptor

Summary by CodeRabbit

  • Refactor

    • Improved clarity and maintainability of translucency effect logic and condition checks.
    • Enhanced enum values and documentation for material model settings.
    • Separated CPU/GPU shared settings from CPU-only settings for better modularity.
  • New Features

    • Added a dynamic mod link for easier access to feature information.
  • Style

    • Updated UI labels and tooltips for alpha modes to be more descriptive and consistent.
    • Revised terminology from "rim light" to "rim edge" throughout the interface.
  • Bug Fixes

    • Adjusted transparency slider to use integer minimum for improved usability.
  • Chores

    • Restoring default settings now explicitly enables the "Skinned Only" option.
    • Simplified settings load/save by handling entire settings struct directly.

- nif setting should ignore SkinnedMeshOnly
- meshes with lightingShaderProperty.alpha < 1 have implicit alpha and
  should not be ignored
  for easy testing

Also clearified the cpu code for translating nif setting into descriptor
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Aug 4, 2025

Walkthrough

The ExtendedTranslucency feature received a refactor to clarify the logic in BSLightingShader_SetupGeometry, especially around property checks and feature descriptor handling. Enum values and UI strings were updated for clarity and consistency. The header was reorganized: new static constants and methods were added, enum semantics were clarified, and string view usage was standardized.

Changes

Cohort / File(s) Change Summary
Shader Geometry Refactor & UI Clarification
src/Features/ExtendedTranslucency.cpp
Refactored geometry setup logic for clarity; improved property checks for translucency; encapsulated feature descriptor logic in a lambda; updated UI labels, tooltips, and slider bounds; revised feature summary text; updated serialization to use Settings including SkinnedOnly.
Header Reorganization & Enum Semantics
src/Features/ExtendedTranslucency.h
Added static MOD_ID and GetFeatureModLink() method; clarified and expanded MaterialModel enum with new values and removed ForceDisabled; renamed MaterialParams to PerFrame and introduced nested Settings struct with CPU-only fields; updated method declarations and string view usage; removed inline helpers and relocated GetFeatureSummary.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant ExtendedTranslucency
    participant Geometry
    participant AlphaProperty
    participant LightingShaderProperty

    Caller->>ExtendedTranslucency: BSLightingShader_SetupGeometry(pass)
    ExtendedTranslucency->>Geometry: Retrieve AlphaProperty and LightingShaderProperty
    alt AlphaProperty with blending enabled OR LightingShaderProperty alpha < 0.999
        ExtendedTranslucency->>ExtendedTranslucency: SetFeatureDescriptor via lambda
        alt ExtraData exists and valid
            ExtendedTranslucency->>ExtendedTranslucency: Remap material model and set descriptor
        else No ExtraData or invalid
            ExtendedTranslucency->>ExtendedTranslucency: Use SkinnedOnly/global defaults
        end
    else
        ExtendedTranslucency->>Caller: Return early (skip effect)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • ArcEarth
  • alandtse

Poem

In the shader's gentle gleam,
A rabbit hops through code's bright stream.
Enums clearer, UI neat,
Translucent edges—what a treat!
With MOD_ID set and tooltips long,
The feature's hop is sure and strong.
🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 88504f6 and 3a0ffab.

📒 Files selected for processing (2)
  • src/Features/ExtendedTranslucency.cpp (3 hunks)
  • src/Features/ExtendedTranslucency.h (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Features/ExtendedTranslucency.cpp
  • src/Features/ExtendedTranslucency.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build plugin and addons
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 4, 2025

Using provided base ref: e39f8cc
Using base ref: e39f8cc
Base commit date: 2025-08-03T17:49:27-07:00 (Sunday, August 03, 2025 05:49 PM)
No actionable suggestions for changed features.

@ArcEarth
Copy link
Copy Markdown
Contributor Author

ArcEarth commented Aug 4, 2025

For testing:
The stellar blade suit, it was not affected by this effect even with the nif extra data, now fixed.
https://mega.nz/folder/ejgGjYZS#h9WXWAJGWYNrAQQDQHo7pw/file/Du5ziIKQ

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 4, 2025

✅ A pre-release build is available for this PR:
Download

@alandtse alandtse changed the title fix (extended translucency): extra data 'AnisotropicAlphaMaterial' does not always work fix(extended translucency): extra data 'AnisotropicAlphaMaterial' does not always work Aug 4, 2025
@alandtse alandtse changed the title fix(extended translucency): extra data 'AnisotropicAlphaMaterial' does not always work fix(extended translucency): extra data 'AnisotropicAlphaMaterial' Aug 4, 2025
Comment thread src/Features/ExtendedTranslucency.cpp Outdated
Comment thread src/Features/ExtendedTranslucency.cpp Outdated
Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
@ArcEarth ArcEarth changed the title fix(extended translucency): extra data 'AnisotropicAlphaMaterial' fix(extended translucency): extra data on NIF not working Aug 5, 2025

struct Hooks;

// TODO: Support more material model like glasses or arcylic
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note Todos will be assigned to you. So determine if you want this or not.

@alandtse alandtse changed the title fix(extended translucency): extra data on NIF not working fix(extended translucency): extra data on NIF Aug 7, 2025
@alandtse
Copy link
Copy Markdown
Collaborator

alandtse commented Aug 7, 2025

Please explain what you meant by "not working" so we can update the title.

@ArcEarth
Copy link
Copy Markdown
Contributor Author

ArcEarth commented Aug 7, 2025

There are 2 case the extra data and the effect not working:

  1. Static mesh with nif extra data setting, but user have Skinned Mesh Only toggled in the setting. That setting should only governs if the Default Material Model being applied. Any NIF mesh marked with extra data should have been tested by mod author and always safe to apply the effect.
  2. Meshes with Alpha in its Nif lighting shader properties are ignored because they may not have a explicit AlphaBlend property.

@alandtse
Copy link
Copy Markdown
Collaborator

alandtse commented Aug 8, 2025

@coderabbitai please suggest an appropriate commit title based off the description of the issues fixed within our length requirements.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Aug 8, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check recent commit titles to understand length requirements and formatting conventions
git log --oneline -20 --format="%s"

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check if we're in a git repository and get basic info
pwd
ls -la
git status
git log --oneline -10

Length of output: 513


🏁 Script executed:

#!/bin/bash
# Try different ways to get commit history
git log --pretty=format:"%s" -10
echo "---"
git log --pretty=oneline -10
echo "---"
# Check remote branches for recent commits
git log origin/main --pretty=format:"%s" -10 2>/dev/null || echo "Could not get origin/main"

Length of output: 29


@alandtse Based on ArcEarth's description of the two issues fixed, I suggest this commit title:

fix(extended translucency): honor nif extra data and implicit alpha

This captures both key fixes:

  1. The feature now honors nif extra data regardless of the "Skinned Mesh Only" setting
  2. It now properly handles meshes with implicit alpha (lightingShaderProperty.alpha < 1)

The title is concise at 60 characters and follows the conventional commit format already used in the current title. It's more descriptive than the current title which only mentions "extra data 'AnisotropicAlphaMaterial'" without explaining what was actually fixed.

Alternative shorter version if length is a concern:

fix(extended translucency): honor extra data and implicit alpha

🧠 Learnings used
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.

Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.796Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.

Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.

Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.

Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-29T00:27:42.717Z
Learning: When reviewing PRs, the committer's description should inform analysis of changes and may take precedence to conclusions when reasonable, but the choice of commit type should still be independently evaluated for compliance with conventional commits. Both committer intent and conventional commit standards should be considered to determine if the classification is defensible.

Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.542Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.

Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.077Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.

@alandtse alandtse changed the title fix(extended translucency): extra data on NIF fix(extended translucency): apply extra data and implicit alpha Aug 8, 2025
@alandtse alandtse merged commit 87a8408 into community-shaders:dev Aug 8, 2025
21 checks passed
@ArcEarth ArcEarth deleted the extended_translucency_fix branch August 8, 2025 12:57
@coderabbitai coderabbitai Bot mentioned this pull request Aug 11, 2025
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.

4 participants