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

Add FogEnvironment modifier #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add FogEnvironment modifier #83

wants to merge 1 commit into from

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Dec 10, 2017

Adds a new Fog Environment modifier to allow per-object fog settings. Multiple objects can reference the same FogEnvironment.

This is implemented by creating a new Blender World for each fog environment and reusing the existing plasma_fni properties. This works without changing the default World associated with the Scene, so the global fog settings remain as they were.

On the subject of global fog settings, if you try to add the global World as a fog environment to an object, it is silently ignored to avoid polluting the PRP files with unnecessary keys. Hypothetically there's a potential optimization where we could avoid adding the fog environment ref if the material is flagged as NoFog/ReallyNoFog.

Worth mentioning that PRP-based FogEnvironments are static and cannot be animated or modified by Python.

@dpogue dpogue requested a review from Hoikas December 10, 2017 23:03
Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

Another review will be incoming when I have a chance to look at the UI in Blender.

@@ -224,6 +224,10 @@ def _create_geospan(self, bo, mesh, bm, hsgmatKey):
if mods.lighting.rt_lights:
geospan.props |= plGeometrySpan.kPropRunTimeLight

# Attach a FogEnvironment if it's non-default
if mods.fogenv.fog_env and mods.fogenv.fog_env != bpy.context.scene.world:
geospan.fogEnvironment = self._exporter().mgr.find_create_key(plFogEnvironment, bl=bo, name=mods.fogenv.fog_env.name)
Copy link
Member

Choose a reason for hiding this comment

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

self._mgr is a shortcut for this

@@ -224,6 +224,10 @@ def _create_geospan(self, bo, mesh, bm, hsgmatKey):
if mods.lighting.rt_lights:
geospan.props |= plGeometrySpan.kPropRunTimeLight

# Attach a FogEnvironment if it's non-default
if mods.fogenv.fog_env and mods.fogenv.fog_env != bpy.context.scene.world:
Copy link
Member

Choose a reason for hiding this comment

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

Considering all the access of mods.fogenv, it would be nice to cache that value for the (small) performance benefit and less verbosity.

"exp2": plFogEnvironment.kExp2Fog
}

class PlasmaFogEnvMod(PlasmaModifierProperties):
Copy link
Member

Choose a reason for hiding this comment

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

Classes are alphabetized


def export(self, exporter, bo, so):
if self.fog_env is None or self.fog_env == bpy.context.scene.world:
return # Don't generate a FogEnv for the default Age FNI fog
Copy link
Member

Choose a reason for hiding this comment

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

Comments should appear on their own lines

fe = exporter.mgr.find_create_object(plFogEnvironment, bl=bo, name=self.fog_env.name)
env = self.fog_env.plasma_fni

fe.type = fog_types[env.fog_method]
Copy link
Member

Choose a reason for hiding this comment

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

The preferred method for doing this is to have blender store the attribute name of the constant and use getattr to apply it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I can do that in this case because the plasma_fni enum values don't match the names of the constants, but are already in use in existing data :(

@@ -168,3 +168,30 @@ def visregion(modifier, layout, context):

# Other settings
layout.prop(modifier, "replace_normal")


Copy link
Member

Choose a reason for hiding this comment

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

Methods should only have one line of whitespace separating them (see PEP8)


col = split.column()
row = col.row()
row.active = fni.fog_method == "linear"
Copy link
Member

Choose a reason for hiding this comment

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

Korman prefers to hide unused properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hiding the start field caused some annoying visual jumping of the end/density fields when toggling between fog types. I tried to work around it by adding a blank space if we didn't need the start field, but apparently Blender ignores that :\

@@ -168,3 +168,30 @@ def visregion(modifier, layout, context):

# Other settings
layout.prop(modifier, "replace_normal")


def fogenv(modifier, layout, context):
Copy link
Member

Choose a reason for hiding this comment

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

Methods should be alphabetized

bl_description = "Adjust per-object fog settings"
bl_icon = "MAT_SPHERE_SKY"

fog_env = PointerProperty(name="Environment",
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a misnomer when you are referencing a world. It's also confusing when accessing bo.plasma_modifiers.fogenv.fog_env because of the redundant naming and inconsistent spelling.

@dpogue
Copy link
Member Author

dpogue commented Dec 11, 2017

Updated. Only thing I didn't address was the lookup table for the attribute names, because I think we need to keep consistency with the existing plasma_fni data that doesn't match those attribute names.

@Hoikas
Copy link
Member

Hoikas commented Dec 22, 2017

I've been sitting on this for awhile because I'm not sure how much I like creating a new ID block in Blender to store fog environments. I'm thinking it might be better to refactor the way fogs are done from the ground up. Namely, instead of having the fog properties stored directly on the world object, we should have a collection of fog properties on the world. I would argue the default fog environment should work like the Default page does, we default to either no fog or D'ni fog but allow the author to override this by creating a nameless or "Default" fog.

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

Successfully merging this pull request may close these issues.

2 participants