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

Option to remove emission from prep materials #369

Merged

Conversation

StandingPadAnimations
Copy link
Collaborator

Changed the branch so reopening

Work on this feature began in Kaion, and now I'm just refining this feature.

This adds an option to prep materials to not add emissive nodes, which can help to reduce noise in Cycles and makes nodes less complex in EEVEE.

@StandingPadAnimations StandingPadAnimations mentioned this pull request Jan 31, 2023
Copy link
Member

@TheDuckCow TheDuckCow left a comment

Choose a reason for hiding this comment

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

Did not live test, just code review. I see your other branches continue to reference needing to merge this first, let me know when you're ready for me to do a further review - but did a first pass just now.

Hence, just responding with "comment" for now instead of approve. Overall the change looks good, just needs a live test from me!

MCprep_addon/materials/generate.py Outdated Show resolved Hide resolved
MCprep_addon/materials/prep.py Show resolved Hide resolved
@StandingPadAnimations
Copy link
Collaborator Author

I've fixed some bugs that were caused because until now I didn't have an easy way to test

The difference is profound

image

image

@StandingPadAnimations
Copy link
Collaborator Author

Looks like prepping on more complex OBJs returns the following error:

Python: Traceback (most recent call last):
  File "C:\Users\Mahid\AppData\Roaming\Bforartists\Bforartists\3.5\scripts\addons\MCprep_addon\tracking.py", line 806, in wrapper
    res = function(self, context)
  File "C:\Users\Mahid\AppData\Roaming\Bforartists\Bforartists\3.5\scripts\addons\MCprep_addon\materials\prep.py", line 252, in execute
    res = generate.matprep_cycles(
  File "C:\Users\Mahid\AppData\Roaming\Bforartists\Bforartists\3.5\scripts\addons\MCprep_addon\materials\generate.py", line 391, in matprep_cycles
    res = matgen_cycles_principled(
  File "C:\Users\Mahid\AppData\Roaming\Bforartists\Bforartists\3.5\scripts\addons\MCprep_addon\materials\generate.py", line 1578, in matgen_cycles_principled
    texgen_seus(mat, passes, nodeInputs, use_reflections)
  File "C:\Users\Mahid\AppData\Roaming\Bforartists\Bforartists\3.5\scripts\addons\MCprep_addon\materials\generate.py", line 1279, in texgen_seus
    links.new(nodeSaturateMix.outputs[saturateMixOut[0]], i)
RuntimeError: Error: Same input/output direction of sockets

@StandingPadAnimations
Copy link
Collaborator Author

Ok that was an easy fix, but I've also noticed that the following occurs:
image

It seems like for some reason MCprep is now putting the B output of Separate RGB to the normal input

I think we need to check if inputs are valid

@StandingPadAnimations
Copy link
Collaborator Author

Ight I was able to fix it with the following:

if use_emission:
    for i in nodeInputs[2]:
        links.new(nodeSeperate.outputs["B"], i)

@TheDuckCow TheDuckCow self-requested a review February 22, 2023 07:04
@TheDuckCow
Copy link
Member

FYI I just using the test runner on this branch, getting some errors, can you take a quick look?

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.2/scripts/addons/MCprep_addon/tracking.py", line 856, in wrapper
    bpy.ops.mcprep.report_error('INVOKE_DEFAULT', error_report=err)
  File "/Applications/blender 3.2/Blender.app/Contents/Resources/3.2/scripts/modules/bpy/ops.py", line 113, in __call__
    ret = _op_call(self.idname_py(), C_dict, kw, C_exec, C_undo)
RuntimeError: Error: Python: Traceback (most recent call last):
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.2/scripts/addons/MCprep_addon/tracking.py", line 827, in wrapper
    res = function(self, context)
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.2/scripts/addons/MCprep_addon/materials/prep.py", line 566, in execute
    res, err = self.update_material(context, mat)
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.2/scripts/addons/MCprep_addon/materials/prep.py", line 633, in update_material
    res = generate.matprep_cycles(
TypeError: matprep_cycles() missing 1 required positional argument: 'use_emission_nodes'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/patrickcrawford/Library/Application Support/Blender/3.2/scripts/addons/MCprep_addon/tracking.py", line 617, in execute
    raise Exception(self.error_report)
Exception:   File "<addon_path>/prep.py", line 566, in execute
    res, err = self.update_material(context, mat)
  File "<addon_path>/prep.py", line 633, in update_material
    res = generate.matprep_cycles(
TypeError: matprep_cycles() missing 1 required positional argument: 'use_emission_nodes'
Location: /Applications/blender 3.2/Blender.app/Contents/Resources/3.2/scripts/modules/bpy/ops.py:113

	TEST FAILED:
	Did not end with finished result

Eevee still benefits from direct to camera shading as emitter, plus
some people may use probes anyways. Also this fixes the redo-last
for use emission, as currently the draw function immediately turns it
back on once you've tried to uncheck it.
Copy link
Member

@TheDuckCow TheDuckCow left a comment

Choose a reason for hiding this comment

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

I'm going to push a single change and then this is good to go. See comment below of one thing I've changed, to not disable emission in Eevee.

I'm going to merge it now.

@@ -150,6 +155,12 @@ def draw_mats_common(self, context):
row = self.layout.row()
row.prop(self, "optimizeScene")

# EEVEE won't benefit from this anyway, so best to disable it
Copy link
Member

Choose a reason for hiding this comment

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

I would argue against this, as having emission means it at least appears brighter even if it doesn't actually give off light. Plus, people can technically use light probes. What harm is there in having emission on in Eevee? I also find that I often switch from eevee to cycles when I first get to rendering as I wasn't thinking about it right away.

emission evidence

@TheDuckCow TheDuckCow merged commit c42face into Moo-Ack-Productions:dev Mar 8, 2023
@StandingPadAnimations StandingPadAnimations deleted the emission-option branch March 8, 2023 17:44
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.

None yet

2 participants