search

pd-externals / earplug

binaural filter based on KEMAR impulse measurement for Pd
GNU General Public License v2.0
14 stars 1 forks source link

remove IR data.txt file #7

Closed chikashimiyama closed 3 years ago

chikashimiyama commented 3 years ago

IR data text file is error-prone, people forget to copy it. the goal is to put all IR data in the header and stop accessing external data from the object.

danomatika commented 3 years ago

Well, it should still load the data file if it's found. The point is that people can override the default dataset, when needed. This should already be working. All we need is the dataset in the header to work.

danomatika commented 3 years ago

See #1 for a related discussion on this. Again, current work is in the develop branch, not main.

chikashimiyama commented 3 years ago

Ok. as you wrote, It is possible to provide a custom IR dataset through the .txt file but then, the file format should be well defined.

chikashimiyama commented 3 years ago

@danomatika

main is one commit above develop that's why I branched from main.

Screenshot 2021-03-30 at 13 53 05

should I merge main to develop and make them the same and branch from develop?

danomatika commented 3 years ago

Ah that’s a mistake about the doc fix. I would work from develop only.

Also, good point about documenting the file format. We should flesh out what is in the readme.

chikashimiyama commented 3 years ago

Ok I won't touch the main branch and cherry pick my pushed change in another branch derived from develop.

umlaeute commented 3 years ago

i think the approach to embed the data in the binary is a wrong one in the first place.

there is needless bloat of the binary just to not have to ship an additional file. the "library" already consists of multiple files (the binary, the help-patch,...), so what?

(a .app bundle is also a directory of many files, typically with binaries and assets separated; i think this is good practice)

chikashimiyama commented 3 years ago

@umlaeute I would not agree with that.

First of all. It's common to embed IR data in VST plugin for example. Juce provides a way to binarize data. The advantage is that binarized data offers a safety net for the users who mishandles the plugin (by accidentally removing the data file).

I know app bundle consists of many files but non-advanced users do not open it. but there is the possibility that the user of earplug just grab and copy the external .pd_darwin binary to his/her project's folder and doesn't know what the additional IR text file means.

Next point is that the bloat in binary would be not significant. the ir data is less than 1 mb. would it be more important to remove 1Mb of redundancy than the safety net?

I think this discussion ultimately goes to how much you trust that the user handle software properly... and obviously, my trust is very limited because I'm developing commercial VST plugins. my view might be pretty biased in the open-source world.

@danomatika

danomatika commented 3 years ago

Why not both? Put a define in to enable/disable building in the default dataset.

chikashimiyama commented 3 years ago

add define check out the PR #14 and merge if it's OK.

danomatika commented 3 years ago

There is now a EARPLUG_DATA_NO_EMBED define which can be set to disable compiling in the data header.