Skip to content

added support for moving source #401

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dzejnwashere
Copy link

Thanks for sending a pull request (PR), we really appreciate that! Before hitting the submit button, we'd be really glad if you could make sure all the following points have been cleared.

Please also refer to the doc on contributing for more details. Even if you can't check all the boxes below, do not hesitate to go ahead with the PR and ask for help there.

  • Are there docstrings ? Do they follow the numpydoc style ?
  • Have you run the tests by doing nosetests or py.test at the root of the repo ?
  • Have you checked that the doc builds properly and that any new file has been added to the repo ? How to do that is covered in the documentation.
  • Is there a unit test for the proposed code modification ? If the PR addresses an issue, the test should make sure the issue is fixed.
  • Last but not least, did you document the proposed change in the CHANGELOG file ? It should go under "Unreleased".

Happy PR 😃

@fakufaku
Copy link
Collaborator

fakufaku commented May 23, 2025

Hi @dzejnwashere , thanks for the contribution. I think there is a possiblity that the room impulse responses (RIR) are actually not recalculated between the steps.
This is not very well documented, but pyroomacoustics stores the room impulse responses, and does not recompute them after the simulation is done. So updating the source locations is not sufficient to guarantee the RIR are updated.
I think this is why the signal scale was also not changed.
I apologize for the poor documentation on that part... 🙇

What I would suggest is to precompute all the source locations in advance, and then simulate all the RIRs at once. This would solve the issue and probably also make the code simpler.

Another thing is that room.py is already a monster at the moment and if possible I would like to avoid adding the moving sources in there.
Also, I don't think it is necessary for the code to be a method of room to work.
At first, would you be ok to change this code to be an example and add in the examples folder?

@fakufaku
Copy link
Collaborator

Finally, the code should be linted with black and isort.
Please consult CONTRIBUTING for instructions.

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.

2 participants