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

Operator to automatically bake lightmaps for selected objects. #281

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

Conversation

GottfriedHofmann
Copy link
Contributor

Added an operator that bakes lightmaps using Cycles for all selected objects that have at least one material. It uses UV slot 1 for the lightmap and creates the slot if not present already. It also automatically creates the required MOZ_Lightmap nodes and wires them correctly. If a MOZ_Lightmap node is present, the addon assumes that the user has taken care of the correct setup and bakes directly.
Baking happens using the Cycles render engine. The created lightmaps are saved as .hdr to Blender's temporary directory, packed and then removed to keep the user's system clean.
The new operator can be found in the Object Properties Editor in a new Panel "Hubs Lightmap Baker"
image

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and apologies for the delay in reviewing it. I think it will provide some good functionality, but I think it's scope should be modified slightly. I think your PR is great for baking light maps for objects that don't have them yet, but your PR doesn't address all the intricacies that can arise when attempting to rebake already set up light maps. PR #48 does this very well and has a lot of error checking already in place, so I'm thinking it makes the more sense to have your PR to bake an initial light map for objects and let the other PR handle any rebaking.

With that in mind, I'm thinking that a check should be added to this PR to avoid baking light maps when they are already present on an object.

PR #48 also adds a panel in the render settings, so I'm thinking that maybe it would be better to add the operator there once that's in, rather than to a new panel in the object properties.

I believe calling bake via execute instead of invoke causes the UI to block with no feedback to the user. I think it would be better to use the non-blocking version and reset all the settings after the bake has completed via a handler (like what is done with reflection probes). The overridden settings can be stored and restored as with reflection probes in reflection_probe.py lines 362-379 and lines 334-337

bpy.app.handlers.object_bake_complete is probably the needed handler.

addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
Comment on lines 739 to 740
for slot in obj.material_slots:
if slot.material not in materials:
Copy link
Contributor

Choose a reason for hiding this comment

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

slot.material should be checked to make sure there is a material present in the slot.

Choose a reason for hiding this comment

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

The condition doesn't check if there are materials used. You may have one material slot but not assigned to material.
Why not just iterating over obj.data.materials here?

for obj in mesh_objs:
    for material in obj.data.materials:
        if material not in materials:

should work correctly.

Choose a reason for hiding this comment

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

Ah actually it may not work, obj.data.materials may give an error
AttributeError: 'PointLight' object has no attribute 'materials'
I had that condition in my code
if obj.type == 'MESH' and getattr(ob.data, "materials", None)
so yeah probably do what @Exairnous suggested:

for obj in mesh_objs:
    for slot in obj.material_slots:
        if slot.material is not None and slot.material not in materials:
            materials.append(slot.material)

Copy link
Contributor Author

@GottfriedHofmann GottfriedHofmann Sep 4, 2024

Choose a reason for hiding this comment

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

Thank you, tested and it works! (luckily I am already testing for mesh objects further up)

addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Show resolved Hide resolved
GottfriedHofmann and others added 6 commits September 3, 2024 17:53
At some point in the Blender 4 series linear was separated out into a bunch of different versions, Linear Rec.709 is what we have used when importing HDRs and seems to be the new default. This code should make it work for all Blender versions

Co-authored-by: Exairnous <[email protected]>
@GottfriedHofmann
Copy link
Contributor Author

I have created this addon with PR #48 in mind.
My addon currently uses the following logic:
Is a material present but no lightmap? - build the lightmap setup
Is a material with lightmap present? - assume the user has set up the light map correctly and re-bake.

bpy.ops.mesh.select_all(action='SELECT')
# TODO: We need to warn the user at some place like the README that the uv_layer[1] gets completely overwritten if it is called 'UV1'
# bpy.ops.uv.lightmap_pack()
bpy.ops.uv.smart_project()

Choose a reason for hiding this comment

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

You create an image texture per material that is not shared between several materials if I'm not mistaken. Don't we need to do the smart_project for each group of objects sharing the same material instead?

I'm commenting on this PR because I'm doing my own Python script from the command line that I started before your PR that I'm continuing now, so I'm comparing with what I have.
I'm using
bpy.ops.uv.smart_project(island_margin=island_margin, correct_aspect=True)
with island_margin=0.03 or 0.001 it depends, that fixed some black artifacts on some of my scene.
Changing bake_settings.margin like you do below may also fix the issue. I just wanted to share that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea with the margin, I think we had some tests where the margin was actually counter productive so I put it into the consts.py config. Now it is bpy.ops.uv.smart_project(island_margin=LIGHTMAP_UV_ISLAND_MARGIN) so you can adjust it to your needs. Might even make sense to include that in the popup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You create an image texture per material that is not shared between several materials if I'm not mistaken. Don't we need to do the smart_project for each group of objects sharing the same material instead?

I have changed the PR to this behavior. On the upside this makes the objects use the available UV space a lot better. On the downside there are some corner cases where we might get overlapping UVs. We should look into combining this with PR #48 to make it work in those cases.

new_filepath = f"//{node.image.name}.hdr"
node.image.packed_files[0].filepath = new_filepath
node.image.filepath_raw = new_filepath
node.image.filepath = new_filepath

Choose a reason for hiding this comment

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

I tested those four lines in my own script on blender 4.2.1, it doesn't seem to be needed if you want to unpack the textures later, the names are correct in the textures folder as far as I can tell, and actually those lines gave me an error during the execution saying the file didn't exist. I didn't test your PR though, so may not apply, but that's something to verify.

…g, uses the space better for multiple objects and materials but is prone to overlaps in some corner cases.
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
obj_list.remove(ob)
self.create_uv_layouts(context, obj_list)
for ob in obj_list:
visited_objects.add(ob)

Choose a reason for hiding this comment

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

This visited_objects logic seems wrong. I spend 1h thinking about it, I hope my example will be clear.

We agree that an object can have several materials, each material with their own image texture, the lightmap uvmap is shared for those materials on an object.

Example:
obj1 with alu and glass materials
obj2 with alu
obj3 with glass
obj4 with rock

We handle alu material first, create uvmap for obj1 obj2 on same image texture for alu.
Then we handle glass material, we don't recreate the uvmap for obj1 because of your visited_objects, we create the uvmap for obj3. The faces for obj1 and obj3 may end up overlapping on the same image texture for glass.

What we need to do is create the uvmap for obj1 obj2 obj3 with the same smart uv project.
For example with alu, glass and rock materials,
iterate over materials, iterating over alu objects, if an object has other materials, include all objects of those materials to do the uv smart project. Mark alu and glass as done. Continue iterating over materials, skip glass because it's done, then handle rock material.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right, it needs to work by material and not by object, should be adressed by a91a5b4

for ob in obj_list:
for slot in ob.material_slots:
if slot.material is not None:
objs_to_uv_unwrap.extend(material_object_associations[slot.material])

Choose a reason for hiding this comment

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

If I'm not mistaken, you may end up having an object several times in objs_to_uv_unwrap. You probably want to use a set and use objs_to_uv_unwrap.update here.

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.

3 participants