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 feature - export particle objects as multimeshinstance to godot #354

Merged
merged 16 commits into from
Nov 13, 2020

Conversation

epth
Copy link
Contributor

@epth epth commented Jul 30, 2020

It has some limitations for now, if a particle-system use a texture to scale particle size,the size in Godot multimesh will be incorrect

@epth
Copy link
Contributor Author

epth commented Jul 30, 2020

This is The Test Blender File, should enable the Particle Enum-Button at the Top while exporting

particle2multimesh_test.zip

Copy link
Contributor

@rcorre rcorre 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 not in charge of this project, but I gave it a quick review since I'm interested in this feature!

I recently had two use cases: scattering lillypads across water and stalactites in a cave. Both were easier to do using particles in blender.

You've got a number of linter warnings, check these out: https://travis-ci.org/github/godotengine/godot-blender-exporter/jobs/714011504#L311

Great work though!

io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/simple_nodes.py Outdated Show resolved Hide resolved
io_scene_godot/converters/__init__.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jason0214 Jason0214 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 very much! This is a great functionality to be added. I have played with it and it works great! I am trying to understand how the particle settings being exported, I am still missing some pieces, so I left some comments. Hope you can help me a little bit and get it merged soon. Thanks again!

io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
def export_multimesh(self, escn_file, export_settings, particle_name):
"""Saves a mesh into the escn file"""
converter = MultiMeshConverter(self.particle_system)
key = MultiMeshResourceKey(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, you can directly use the bpy ParticleSystem as the hash key for MultiMeshResource. Just like what we did with shader. Every bpy project is hashable and has a unique hash value.
The reason that we build a custom hash key for the MeshResource is that bpy mesh may need applying modifiers when being exported, which means a reference to the same bpy mesh might be exported to different MeshResources. Therefore, modifier attributes are included in MeshResource's hash key.
I think this is not the case with MultiMesh, correct me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above,Because if "Render-Render As" are switched to "Collection",there maybe several objects instanced in one particle system.so when we choose one of particle system,maybe different object should be instance,I admit just use hash from Bpy is good for now. I thought too much maybe give an option that can select which object in "collection" to export in future haha.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good point, if in the future we have custom logic for each object referring an instanced particle system to export to a MutltiMeshInstance, then a bpy ParticleSystem hash might not be enough.

I am okay with current implementation and I think it works well. However, I still feel that the MultiMeshResourceKey is a little bit overcomplicated. In my opinion, make the key refer to bpy ParticleSystem object instead of the particle system name is better. The later is guaranteed unique. The gd_rsc_type seems unnecessary, only MultiMesh is involved here.

Anyway I will merge it whether you change it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks and not change for now!

io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
io_scene_godot/converters/multimesh.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jason0214 Jason0214 left a comment

Choose a reason for hiding this comment

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

Btw, could you also fix up the pycodestyle warning as well. Really appreciate it. : )

pycodestyle io_scene_godot
io_scene_godot/structures.py:425:25: E231 missing whitespace after ','
io_scene_godot/converters/multimesh.py:17:23: E225 missing whitespace around operator
io_scene_godot/converters/multimesh.py:26:80: E501 line too long (82 > 79 characters)
io_scene_godot/converters/multimesh.py:31:80: E501 line too long (80 > 79 characters)
io_scene_godot/converters/multimesh.py:37:1: W293 blank line contains whitespace
io_scene_godot/converters/multimesh.py:38:13: E225 missing whitespace around operator
io_scene_godot/converters/multimesh.py:39:31: E225 missing whitespace around operator
io_scene_godot/converters/multimesh.py:175:55: E231 missing whitespace after ','
io_scene_godot/converters/multimesh.py:175:65: E231 missing whitespace after ','
Makefile:12: recipe for target 'pep8' failed

You can check the Travis CI status, see https://travis-ci.org/github/godotengine/godot-blender-exporter/jobs/741590402

def export_multimesh(self, escn_file, export_settings, particle_name):
"""Saves a mesh into the escn file"""
converter = MultiMeshConverter(self.particle_system)
key = MultiMeshResourceKey(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good point, if in the future we have custom logic for each object referring an instanced particle system to export to a MutltiMeshInstance, then a bpy ParticleSystem hash might not be enough.

I am okay with current implementation and I think it works well. However, I still feel that the MultiMeshResourceKey is a little bit overcomplicated. In my opinion, make the key refer to bpy ParticleSystem object instead of the particle system name is better. The later is guaranteed unique. The gd_rsc_type seems unnecessary, only MultiMesh is involved here.

Anyway I will merge it whether you change it or not.

@Jason0214
Copy link
Collaborator

Thank you very much!It looks better now.
Though we still get some code style warning from pycodestyle. The rules are rigid 😆, but we have been using it until now, it is easier to fix the code a little bit to make the linter happy.

  • You can make variable names a little bit longer and meaningful
  • For the duplicated code, if you can not make it pass, I am okay with just disable it. Check the other examples in the code, I think you can find out how to do it.
    Btw, you can click the ❌ or ✔️ near your every commit to see the Travis CI build log.
    Really appreciate your effort, I think we are very clock to merge 🎉

@epth
Copy link
Contributor Author

epth commented Nov 10, 2020

@Jason0214
Glad to know will be merge, but have no idea with how to fix pep8 error due to duplicated code.

@Jason0214
Copy link
Collaborator

My bad. Looks like disable pylint's duplicate-code disable is broken : (
I pushed a commit to just using bpy ParticleSystem as the hash key, it is not the most robust one, but enough be working for now. Don't have enough time to wait for the CI build. I will check it later.
If you are okay with the change, I will merge this PR after CI is fixed.

@epth
Copy link
Contributor Author

epth commented Nov 11, 2020

My bad. Looks like disable pylint's duplicate-code disable is broken : (
I pushed a commit to just using bpy ParticleSystem as the hash key, it is not the most robust one, but enough be working for now. Don't have enough time to wait for the CI build. I will check it later.
If you are okay with the change, I will merge this PR after CI is fixed.

Im ok, you can merge it !

@Jason0214 Jason0214 merged commit a01b0e9 into godotengine:master Nov 13, 2020
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.

None yet

4 participants