blender/blender-asset-tracer
2
5
Fork 5
Code Issues 3 Pull Requests 1 Projects Releases Wiki Activity

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
Conversation 10 Commits 3 Files Changed 10 +1913 -1
Olivier C commented 2023-12-08 11:33:02 +01:00
Contributor
Copy Link

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
Copy Link

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.
Olivier C commented 2023-12-15 10:30:39 +01:00
Author
Contributor
Copy Link

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
Olivier C commented 2023-12-15 11:07:47 +01:00
Author
Contributor
Copy Link

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
Sybren A. Stüvel commented 2023-12-18 18:00:39 +01:00
Member
Copy Link

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?
Sybren A. Stüvel commented 2024-01-02 17:10:31 +01:00
Member
Copy Link

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.
ariane-genty commented 2024-01-02 18:02:20 +01:00
First-time contributor
Copy Link

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.
ies_scene.blend
950 KiB
Sybren A. Stüvel commented 2024-01-04 11:53:46 +01:00
Member
Copy Link

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.
ariane-genty commented 2024-01-05 09:43:09 +01:00
First-time contributor
Copy Link

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.
Sybren A. Stüvel commented 2024-01-05 11:35:10 +01:00
Member
Copy Link

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
Olivier C commented 2024-01-10 16:30:02 +01:00
Author
Contributor
Copy Link

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
Sybren A. Stüvel commented 2024-01-11 16:32:35 +01:00
Member
Copy Link

Thanks for the extra effort!

Thanks for the extra effort!
❤️ 1
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.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
legacy module
Platforms, Builds, Tests & Devices

  
legacy project
Blender Asset Tracer

  
legacy project
Platform: Windows

  
Priority::Normal

   
Status::Archived

   
Status::Confirmed

   
Status::Needs Triage

   
Status::Resolved

   
Type::Design

   
Type::Report

   
Type::To Do
No Label
legacy module
Platforms, Builds, Tests & Devices
legacy project
Blender Asset Tracer
legacy project
Platform: Windows
Priority::Normal
Status::Archived
Status::Confirmed
Status::Needs Triage
Status::Resolved
Type::Design
Type::Report
Type::To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender-asset-tracer#92886
No description provided.
Delete Branch "oliverpool/blender-asset-tracer:fix_ies_blocks2assets"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?