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

Fix update bug of LaserMaterial #421

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

Mateasek
Copy link
Member

Fixes problem described in #420

In short:
Laser primitives are now dropped and recreated after Laser.transform changes which removes the problem of old transfomrs being cached in LaserMaterial

Laser primitives have to be rebuild with the materials.
Possible source of the problem:
Laser node is the parent of the laser primitives
After laser.transform is changed the _modified method is called before
the transforms of the laser primitives is changed because the Laser node
is above the laser primitives in the scenegraph.
Copy link
Member

@vsnever vsnever left a comment

Choose a reason for hiding this comment

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

Hi @Mateasek, nice catch!

Ideally, if a material caches geometry of the primitive, any change in the geometry should automatically trigger cache rebuild. However, I don't know how to implement this, because laser segments are just basic primitives with an empty _modified() method. Considering this, your solution is fine.

@vsnever
Copy link
Member

vsnever commented Oct 27, 2023

Just as an option, wouldn't it be safer in general to cache affine matrices the first time the emission_function() is called, rather than when the LaserMaterial is initialised? In this case, the LaserMaterial must keep the references to laser and laser segment nodes, but the Laser does not have to rebuild its internal geometry when the node is transformed.

@Mateasek
Copy link
Member Author

Hi @vsnever

thanks for looking into it. The reaction to the material change would mean that the laser primitives couldn't be regular Raysect primitives, which I would like to avoid.

I like the idea of caching the transforms in the emission function. It is indeed the safest option. Thanks for suggesting it. I will apply the changes. I don't think that rebuilding the primitives on every change is a big deal, but from the point of view of safety your idea is much better.

The transforms are cached first time the emission method is called
This should be a safer option
The LaserMaterial now can be assigned only to a single primitive
@Mateasek
Copy link
Member Author

As suggested by @vsnever the transforms are now checked and cached in the emission function.

Also I added a check for the number of primitives the material is assigned to. Because of the transform caces the material should be assigned to only a single primitive for safety.

@Mateasek Mateasek requested a review from vsnever October 27, 2023 09:43
Copy link
Member

@vsnever vsnever left a comment

Choose a reason for hiding this comment

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

Thanks @Mateasek, I like this fix better. Could you please merge the development branch into this, because it now has a fix for #422?

@Mateasek
Copy link
Member Author

@vsnever, I merged the latest changes from development.

Copy link
Member

@jacklovell jacklovell left a comment

Choose a reason for hiding this comment

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

Looks OK to me. Thanks Matej!

@jacklovell jacklovell merged commit 0e6a150 into cherab:development Nov 30, 2023
6 of 8 checks passed
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.

3 participants