moderngl / moderngl-window

A cross platform utility library for ModernGL making window creation and resource loading simple
MIT License
245 stars 59 forks source link

VAO instance cache may be invalid #191

Open julee opened 8 months ago

julee commented 8 months ago

The VAO instance method uses the program id as a key to cache VertexArray internally.

However, if the original Program is released, and a new Program is created, the opengl id of the new Program may be the same as the original program.

But the vertex attributes of the new Program have a different layout. At this time, calling the VAO instance method to get the VertexArray from the cache will be wrong!

This is a potential error behavior, difficult to be discovered.

A better way might be to remember the layout of the vertices, see if they are consistent, and then decide whether to reuse the VertexArray in the cache.

Source code with potential issues:

    def instance(self, program: moderngl.Program) -> moderngl.VertexArray:
        """Obtain the ``moderngl.VertexArray`` instance for the program.

        The instance is only created once and cached internally.

        Args:
            program (moderngl.Program): The program

        Returns:
            ``moderngl.VertexArray``: instance
        """
        vao = self.vaos.get(program.glo)
        if vao:
            return vao
einarf commented 8 months ago

Right. I guess that is driver dependent. More data needs to be added to the key.

Potential key values

This is a semi-hot path, so it should ideally be as light as possible.

einarf commented 8 months ago

It could also add some unique identifier in the Program.extra dict. That might be the best solution

julee commented 8 months ago

It could also add some unique identifier in the Program.extra dict. That might be the best solution

Program.extra is for user use, it seems inappropriate to use it here. Your suggestion of using "The id() of the object" may be a better idea.

einarf commented 8 months ago

Python's id() can have the same problem as using program.glo. I've had problems with this in the past.

julee commented 8 months ago

Python's id() can have the same problem as using program.glo. I've had problems with this in the past.

Yes, I forgot it. Perhaps we can use weakref to track whether the program object still exists, if it exists and the id is the same, we can confirm that the program matches.

The use of Program.extra that you mentioned might be the simplest method, but from an interface design perspective, extra should be for the user to use, and the user could overwrite it with a new object at any time.

Similarly, adding an additional identifier member in Program might also be a way. Perhaps this is the most intuitive way. But this requires modifications in moderngl.

einarf commented 1 day ago

WeakValueDictionary is definitely the only solution here as far as I can see.