Fix IES lights blocks2assets #92886

Merged
Sybren A. Stüvel merged 3 commits from oliverpool/blender-asset-tracer:fix_ies_blocks2assets into main 2024-01-11 16:34:29 +01:00
Contributor

If the IES file is outside the project folder, BAT currently fails with

KeyError: "Struct(b'Lamp') has no field b'filepath', only [b'_pad0', b'_pad2', b'_pad6', b'adt', b'area_shape', b'area_size', b'area_sizey', b'area_sizez', b'area_spread', b'att1', b'att2', b'att_dist', b'b', b'bias', b'bleedbias', b'bleedexp', b'buffers', b'bufflag', b'bufsize', b'buftype', b'cascade_count', b'cascade_exponent', b'cascade_fade', b'cascade_max_dist', b'clipend', b'clipsta', b'coeff_const', b'coeff_lin', b'coeff_quad', b'contact_bias', b'contact_dist', b'contact_spread', b'contact_thickness', b'curfalloff', b'diff_fac', b'dist', b'energy', b'falloff_type', b'filtertype', b'flag', b'g', b'id', b'ipo', b'k', b'mode', b'nodetree', b'pr_texture', b'preview', b'r', b'samp', b'shadhalostep', b'shdwb', b'shdwg', b'shdwpad', b'shdwr', b'soft', b'spec_fac', b'spotblend', b'spotsize', b'sun_angle', b'texact', b'type', b'use_nodes', b'volume_fac']"

This is likely because result.BlockUsage expects the storage block and not the Lamp block.

If the IES file is outside the project folder, BAT currently fails with ``` KeyError: "Struct(b'Lamp') has no field b'filepath', only [b'_pad0', b'_pad2', b'_pad6', b'adt', b'area_shape', b'area_size', b'area_sizey', b'area_sizez', b'area_spread', b'att1', b'att2', b'att_dist', b'b', b'bias', b'bleedbias', b'bleedexp', b'buffers', b'bufflag', b'bufsize', b'buftype', b'cascade_count', b'cascade_exponent', b'cascade_fade', b'cascade_max_dist', b'clipend', b'clipsta', b'coeff_const', b'coeff_lin', b'coeff_quad', b'contact_bias', b'contact_dist', b'contact_spread', b'contact_thickness', b'curfalloff', b'diff_fac', b'dist', b'energy', b'falloff_type', b'filtertype', b'flag', b'g', b'id', b'ipo', b'k', b'mode', b'nodetree', b'pr_texture', b'preview', b'r', b'samp', b'shadhalostep', b'shdwb', b'shdwg', b'shdwpad', b'shdwr', b'soft', b'spec_fac', b'spotblend', b'spotsize', b'sun_angle', b'texact', b'type', b'use_nodes', b'volume_fac']" ``` This is likely because `result.BlockUsage` expects the `storage` block and not the `Lamp` block.
Olivier C added 1 commit 2023-12-08 11:33:03 +01:00
Sybren A. Stüvel requested changes 2023-12-14 10:39:01 +01:00
Sybren A. Stüvel left a comment
Member

Could you add a unit test for this as well? This will likely need to have some example file in tests/blendfiles to accompany it.

Could you add a unit test for this as well? This will likely need to have some example file in `tests/blendfiles` to accompany it.
Author
Contributor

Sure, how can I run the tests ? (I couldn't find any doc regarding them)

Sure, how can I run the tests ? (I couldn't find any doc regarding them)
Olivier C force-pushed fix_ies_blocks2assets from 68483c5edd to 54fd4d6aea 2023-12-15 11:06:46 +01:00 Compare
Author
Contributor

Found out that running poetry run pytest -k test_pack_ies_external --no-cov was what I wanted.

I rebased on top of main and reordered the commits to show that running the test above on 0ad1e0f fails, while it succeeds on 54fd4d6

Found out that running `poetry run pytest -k test_pack_ies_external --no-cov` was what I wanted. I rebased on top of main and reordered the commits to show that running the test above on 0ad1e0f fails, while it succeeds on 54fd4d6
Olivier C requested review from Sybren A. Stüvel 2023-12-15 17:28:58 +01:00

This PR contains two (identical) IES files. Where did they come from, and what is their license?

This PR contains two (identical) IES files. Where did they come from, and what is their license?

I can confirm that this PR fixes the issue, and that the unit test is indeed sufficient.

This question is still open:

This PR contains two (identical) IES files. Where did they come from, and what is their license?

This is important info to know, as the file will become part of the Flamenco sources. This PR cannot move forward without knowing the license of the attached file.

I can confirm that this PR fixes the issue, and that the unit test is indeed sufficient. This question is still open: > This PR contains two (identical) IES files. Where did they come from, and what is their license? This is important info to know, as the file will become part of the Flamenco sources. This PR cannot move forward without knowing the license of the attached file.
First-time contributor

The IES file comes from ies library
Previews are under Creative Commons Licence, is it suitable ?

But I noticed the scene is not correctly set, I corrected it (2 lights addressing the IES relative to the project vs outside project),
you will find the new scene in attachment, but if you prefer, I can create a new PR.

The IES file comes from [ies library](https://ieslibrary.com/browse#ies-0b6099e26a80d7d79da7a78d1853224c) Previews are under [Creative Commons Licence](https://ieslibrary.com/help/licenses), is it suitable ? But I noticed the scene is not correctly set, I corrected it (2 lights addressing the IES relative to the project vs outside project), you will find the new scene in attachment, but if you prefer, I can create a new PR.

The IES file comes from ies library
Previews are under Creative Commons Licence, is it suitable ?

That's the license of the preview images, and not the IES file itself. That page also tells us:

IES and LDT files are not licensed as they can be downloaded free of charge from the manufacturer's website.
These files are not a work of art, they are only the result of a light test and are provided by the manufacturer to simulate a light source.

I don't know if "not licensed" is enough here; AFAIK depending on local laws it might actually imply "fully restricted under default copyright law" because there is no explicit license that says otherwise. The fact that we can download the files for free, does not mean the freedom to redistribute those files. Also the text seems to imply that the files originate from a third party ("the manufacturer").

For me this is all too vague to be confident we can include the file in the Flamenco sources.

But I noticed the scene is not correctly set, I corrected it (2 lights addressing the IES relative to the project vs outside project),
you will find the new scene in attachment, but if you prefer, I can create a new PR.

There is no need to create a new PR, but please do update its contents. @oliverpool can just push another commit to the same branch.

> The IES file comes from [ies library](https://ieslibrary.com/browse#ies-0b6099e26a80d7d79da7a78d1853224c) > Previews are under [Creative Commons Licence](https://ieslibrary.com/help/licenses), is it suitable ? That's the license of the preview images, and not the IES file itself. That page also tells us: > IES and LDT files are not licensed as they can be downloaded free of charge from the manufacturer's website. > These files are not a work of art, they are only the result of a light test and are provided by the manufacturer to simulate a light source. I don't know if "not licensed" is enough here; AFAIK depending on local laws it might actually imply "fully restricted under default copyright law" because there is no explicit license that says otherwise. The fact that we can download the files for free, does not mean the freedom to redistribute those files. Also the text seems to imply that the files originate from a third party ("the manufacturer"). For me this is all too vague to be confident we can include the file in the Flamenco sources. > But I noticed the scene is not correctly set, I corrected it (2 lights addressing the IES relative to the project vs outside project), > you will find the new scene in attachment, but if you prefer, I can create a new PR. There is no need to create a new PR, but please do update its contents. @oliverpool can just push another commit to the same branch.
First-time contributor

Unfortunately, @oliverpool is working on other projects right now, so I will try to complete this PR for him.

But I don't have rights to update his fork, so I opened a new PR : sorry about that,...

I found a GPL IES package here : I hope it solves this.

Unfortunately, @oliverpool is working on other projects right now, so I will try to complete this PR for him. But I don't have rights to update his fork, so I opened a new PR : sorry about that,... I found a GPL IES package [here ](https://leomoon.com/store/shaders/ies-lights-pack/): I hope it solves this.

Unfortunately, @oliverpool is working on other projects right now, so I will try to complete this PR for him.

That's unfortunate, as it makes it hard to collaborate on things like this.

But I don't have rights to update his fork, so I opened a new PR : sorry about that,...

At least link to the new PR? If it is #92888, please take the time to write a description so that it is clear what it is about. This isn't just a quick one-on-one between the two of us, but a historical record of how and why changes were made to Flamenco. Proper descriptions, and tracability of decision making, are quite important.

I found a GPL IES package here : I hope it solves this.

That's a lot better, thanks.

> Unfortunately, @oliverpool is working on other projects right now, so I will try to complete this PR for him. That's unfortunate, as it makes it hard to collaborate on things like this. > But I don't have rights to update his fork, so I opened a new PR : sorry about that,... At least link to the new PR? If it is #92888, please take the time to write a description so that it is clear what it is about. This isn't just a quick one-on-one between the two of us, but a historical record of how and why changes were made to Flamenco. Proper descriptions, and tracability of decision making, are quite important. > I found a GPL IES package [here ](https://leomoon.com/store/shaders/ies-lights-pack/): I hope it solves this. That's a lot better, thanks.
Olivier C force-pushed fix_ies_blocks2assets from 54fd4d6aea to 176af17445 2024-01-10 16:27:20 +01:00 Compare
Author
Contributor

Hi, I was away for vacations. I will try to help get this PR through.

I reworked my PR:

Running poetry run pytest -k test_pack_ies_external --no-cov shows that the added test initially fails and gets fixed by the latest commit.

Hi, I was away for vacations. I will try to help get this PR through. I reworked my PR: - rebased on main (to resolve the conflicts) - the test commit now uses the file shared by @ariane-genty in #92888 : 531e2784a13bc68cd98c09b58b430cfef3c0e151 - the fix on top: 176af174459c2343c47cd199313e60be0d77ee41 Running `poetry run pytest -k test_pack_ies_external --no-cov` shows that the added test initially fails and gets fixed by the latest commit.
Olivier C added 1 commit 2024-01-10 16:32:24 +01:00
Olivier C force-pushed fix_ies_blocks2assets from 532856a550 to 118353ed0b 2024-01-10 16:34:15 +01:00 Compare

Thanks for the extra effort!

Thanks for the extra effort!
Sybren A. Stüvel merged commit 44d1d2ff90 into main 2024-01-11 16:34:29 +01:00
Sybren A. Stüvel deleted branch fix_ies_blocks2assets 2024-01-11 16:34:30 +01:00
Sign in to join this conversation.
No description provided.