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

Primera versión del gestor de recursos. #58

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ArnauPrat
Copy link

Implementación de un TextureManager usando el template ResourceManager, como ejemplo.
Modificada App para que cargue una textura a modo de ejemplo.

Implementación de un TextureManager usando el template ResourceManager, como ejemplo.
Modificada App para que cargue una textura a modo de ejemplo.
/**
* @brief deinicializa el gestor de recursos.
**/
void deinit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Yo pondría el nombre release, dispose o algo así, prefiriendo en este caso dispose. deinit no lo había oído nunca, aunque he visto que existe por ahí. Pero dispose tiene un significado mucho más claro.

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

Que os parece hacer que el gestor de recursos vaya a una nueva rama develop hasta que esté completo? Creo que aún tiene bastante que madurar y cambiar porque va a ser parte importante del engine.


private:

std::map< std::string, T* > resources; /**< @brief Mapa que mapea strings a recursos. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Este comment debería ir en la línea de arriba ¿no?

@ArnauPrat
Copy link
Author

Estoy de acuerdo con @adrigm dado que es algo que va para largo y creo que es clave para el proyecto.
Respecto a lo que comenta @rickyah , siempre tengo 1000 problemas para dar nombre a la funcion deinit/release o como sea. En este caso usé deinit por una simple cuesitión de simetria y coherencia con alguna otra clase ya existente en el proyecto ( que usa init para inicializar ). Yo normalmente uso StartUp/ShutDown...

@rickyah
Copy link
Contributor

rickyah commented Nov 10, 2013

Al respecto, creo que estaría bien definir una convención concreta para nombrar a los métodos init / release, y aplicarlo siempre
Yo indicaba la de dispose, por que es la que usa C#. Ya voy avisando que soy bastante fan del lenguaje, me parece muy bien pensado (que no perfecto) pero este es uno de los casos en los que no importa tanto qué nombre elijamos mientras lo apliquemos de forma consistente

for( typename std::map< std::string, T* >::iterator it = resources.begin(); it != resources.end(); it++ )
{
deinitResource( (*it).second );
delete (*it).second;
Copy link
Contributor

Choose a reason for hiding this comment

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

Creo que ya borras el recurso dentro de deinitResource, ¿lo puedes confirmar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ni caso, me he equivocado con freeResource

Copy link
Author

Choose a reason for hiding this comment

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

No,
Mi idea inicial es que todo lo referente allocatación y manejo de la memoria esté implementado en ResourceManager. Este, contiene dos funciones virtuales puras, initResource y deinitResource. initResource es la encargada de implementar como se carga el recurso en cuestión. deinitResource en principio esta por si antes de deallocatar la memória del recurso, hubiera que liberar algo mas relacionado a el, me explico?

Como puedes ver, ahora mismo se hace new y delete dentro del ResourceManager. Inicialmente, mi idea era simplemente reservar la memória, y dentro de initResource/deinitResource llamar a un placement new/delete. De esta manera, no estariamos atados a ningun paradigma de programación. En cambio, si el recurso necesita a la fuerza un parametro en su constructora, no compilaria. Lo veis muy liado?

Copy link
Contributor

Choose a reason for hiding this comment

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

Si no lo entiendo mal de/initResource son un (Template Method Pattern)[http://en.wikipedia.org/wiki/Template_method_pattern]

Me he bajado la rama a mi local y viéndolo en el editor ya me queda más claro.

Copy link
Author

Choose a reason for hiding this comment

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

Efectivamente, seria eso. También puede ser que los nombres escogidos no sean los mejores :)

Yo lo que intento con mi implementación, ahora mismo, és lo sigüiente:

  1. Aprovechar el máximo posible el código para ahorrar tiempo en implementaciones y posibles bugs
  2. Que la gestión de la memória recaiga absolutamente en el gestor. El objetivo, es que, en un futuro, si distintos tipos de recursos requieren distintas politicas de alocatación, estas se puedan definir con allocators.
  3. Intentar que sea lo mas flexible posible, es decir, que por un lado puedas usar recursos independientemente del paradigma de inicializacion usado (ya sea con constructor o con funciones si el recurso es POD), y por el otro, que no ate al usuario del motor a los recursos predefinidos en este. Si el usuario para su juego necesita un recurso no soportado por el motor, que pueda crear un gestor de recursos rapidamente a partir del que proporcionemo nosotros.

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

yo utilizo init y cleanup, es como está implementado en App y en el SceneManager. En señal de inicialización y tareas de limpieza.

@ArnauPrat
Copy link
Author

Pues propongo usar como convencion el formato init/cleanUp que usa @adrigm . Estaria bien actualizar la wiki.

@rickyah
Copy link
Contributor

rickyah commented Nov 10, 2013

Acabo de actualizar la wiki con esa convención

int size = this->resources.size();
for( typename std::map< std::string, T* >::iterator it = resources.begin(); it != resources.end(); it++ )
{
deinitResource( (*it).second );
Copy link
Contributor

Choose a reason for hiding this comment

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

aquí debería ser this->deinitResource()

Bueno, ahora según la convención que hemos discutido sería this->cleanUpResource() pero me refiero a que hay bastantes llamadas a métodos internos que no están usando el this->

Copy link
Author

Choose a reason for hiding this comment

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

OK, le hago una repasada.

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

@ArnauPrat, A mi lo que no me acaba de convencer es tener un objeto para cada tipo de recurso, es decir un textureManager, soundManager, ...

No sería mejor algo integrado?

@rickyah
Copy link
Contributor

rickyah commented Nov 10, 2013

Más que un ResourceManager en realidad es un Resource.

Es decir, en mi opinión, habría que tener un ResourceManager que gestiona todos los recursos, y luego una clase base virtual Resource, que los únicos métodos que tendría serían para cargar eliminar el recurso.

Luego, para cada recurso que queramos añadir, simplemente hay que hacer una clase derivada de Resource para gestionar cómo debe gestionarse dicho recurso (cargar los datos, o hacer un release)

El ResourceManager entonces sólo se encarga de gestionar, comprueba que un recurso no se carga dos veces, etc, pero no sabe nada de cómo cargarlo, ni siquiera sabe el tipo de dato que carga.

Esta es la idea:

resorces structure

Esto ya está casi así de hecho.

@desclapez
Copy link

Buenas, antes de nada, buen trabajo! Esta es la primera vez que comento y me gustaría hacer algunos apuntos en cuanto al API de esta clase.

Creo que añadir el nombre Resource a cada método es algo redundante, puesto que la clase ya se llama ResourceManager podría omitirse y así acortar los nombres, en mi opinión ya se sobre entienden.

Propongo esta interfaz:

ResourceManager
public:
    - init()
    - cleanUp()
    - get(string fileName);
    - delete(string fileName);

protected:
        - T* load(string fileName)
    - void release(T* resource)

Un ejemplo:

ResourceManager resourceManager = new ResourceManager();
resourceManager.init();


Resource *resource = resourceManager.get('Data/textures/bird.jpg')
...

resourceManager.delete('Data/textures/bird.jpg')

...


resourceManager.cleanUp();

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

@rickyah, tú método es lo que había pensado, pero como hacemos que se nos devuelva un recurso? Me explico:

Tu tienes tu clase Abstracta Resource que heredan todos los tipos de recursos y que tendrá sus métodos virtuales puros Load, Cleanup, ... que cada recurso implementará convenientemente, hasta ahí bien.

Ahora como gestionas el recurso pedido para que devuelva un recurso adecuado, no puedes tener un método Get genérico ya que cada recurso devolverá un tipo de dato distinto.

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

Yo hago mi propuesta que era algo así:

https://gist.github.com/adrigm/7405117

Es algo rudimentario, pero creo que es simple. Su utilización sería:

AssetManager* am = AssetManager::instance();
sf::Texture *t = am->getTexture("data/player.png");

El AssetManager se encargaría internamente de comprobar si ya está cargada y devolver el puntero adecuado, o si no, cargarla y devolver el puntero. En caso de no poder cargarla podría devover una textura vacía por defecto impidiendo que falle el engine.

@desclapez
Copy link

Yo así lo veo perfecto @adrigm y pondría el path en el init.

Por cierto los parámetros con el "the" delante entran en la guía de estilo? Es que he visto que por el ejemplo la clase Log no utiliza esta "convención"

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

@titoneo, es de un antiguo proyecto mío en lo que lo usaba. Como veáis si lo metemos en la guía o lo descartamos para todas las clases.

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

Podríamos include tener algo como

addPath("...")

para que el usuario añada varias carpetas donde quiere buscar recursos

@rickyah
Copy link
Contributor

rickyah commented Nov 10, 2013

Con templates y castings se puede hacer bien, y es más extensible que la solución que propones. Estoy haciendo un POC

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

@rickyah, en que sentido sería más extensible?

@rickyah
Copy link
Contributor

rickyah commented Nov 10, 2013

Generas tu clase Resource que sepa como cargarlo, y ya puedes usarlo en el resource manager.
Con tu idea hay que añadir un nuevo método por cada recurso, y que el ResourceManager se encargue de gestionar los recursos y además de cargarlos borrarlos según el tipo de recurso podría ser visto como tener dos responsabilidades.

Yo de todas maneras acabo mi idea y os la pongo por un gist, guste o no, se meta o no al final, algo aprenderé y así lo debatimos, que al final es de lo que va todo esto :)

@adrigm
Copy link
Contributor

adrigm commented Nov 10, 2013

@rickyah Sí, esa es la pega de mi método que si se quieren añadir nuevos recursos hay que tocar el AssetManager. Veamos la que nos planteas tú!

@rickyah
Copy link
Contributor

rickyah commented Nov 10, 2013

Puede que tarde un poco, que tengo c++, y sobre todo las templates oxidadas :(

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

Aquí está el gist con mi prueba de concepto. Está hecho mal y pronto, pero es para mostraos qué os parece antes de irme a la cama

https://gist.github.com/rickyah/7a9a63bdfdaf2e3dbcd4

@ArnauPrat
Copy link
Author

Buenos dias,
La arquitectura que proponeis, está bien. Pero cuando pensé en como hacer el gestor de recursos, la descarté principalmente por los siguientes motivos:

  • Dar la responsabilidad de gestionar la memória al propio gestor (y no al cargador del recurso), creo que a la larga es mas potente. En mi propuesta, el tamaño del recurso es conocido estáticamente (en tiempo de compilación), con lo que se pueden crear estructuras mas compactas, o que el compilador optimize mas. Además, damos la posibilidad a que cada recurso pueda tener una política de alocatamiento distinta, en función de sus necesidades, si se requiere. Esto se haria pudiendo pasar alocatadores al gestor de recursos, que definieran la política de alocatado. ( Esto creo que en vuestra propuesta se podria llegar a añadir, concretamente a la clase abstracta Resource )
  • No le veo ninguna ventaja a tener el gestor de recursos unificado, mas alla del no tener que recordar los nombres. A parte, si en algun momento tenemos que usar HashedStrings para reducir el tiempo en las comparaciones, tener los gestores separados reduciria los posibles conflictos.
  • Tampoco veo que en vuestra propuesta la cantidad de código a implementar sea menor. El número de funciones a sobrecargar sigue siendo el mismo, 2.
  • Vuestra propuesta añade más verbose al código, dado que se requiere llamar a getData, mientras que en la mia directamente se devuelve el objeto del tipo requerido.

Aun así, he de reconocer que no son diferencias cruciales/determinantes, así que si lo creeis conveniente, adelante.

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

Mi opinión a tus consideraciones:

  • Lo lógico, para mi, es que el recurso sea el que gestione la memoria, ya que él es el que sabe lo que tiene que cargar y como. De hecho tu propuesta incluye una función para sobrecargar en el gestor por si acaso hay que hacer alguna cosa "rara" con algún recurso. No se qué ventaja tiene conocer el tamaño del recurso en tiempo de compilación: eso es irrelevante por que los datos del recurso (e.g. lo que ocupa una imagen, sonido) es imposible saberlo en tiempo de compilación, y es realmente lo que es necesario.
  • Tener un gestor de recursos unificado hace más sencillo de gestionar, si no necesitaremos crear una instancia por cada tipo de recurso que queramos usar. Eso implica que si añadimos un nuevo tipo de recurso tenemos que crear una nueva instancia del resource manager en el código. Con esta propuesta no es necesario. También nos permitiría hacer cambios más fácilmente. Por ejemplo si en el futuro queremos agrupar los recursos del mismo tipo en memoria para maximizar los hits de cache, o poder utilizar diferentes estructuras de datos internas para guardar diferentes tipos de resource, podríamos hacerlo sin problema, cambiando sólo el interior del resource manager y sin cambiar el resto del código que lo usa. Por último separar los recursos del gestor cumple con el SRP.
  • Nadie dijo que el código a implementar fuera menor :P Pero tampoco es mucho mayor.
  • Lo de añadir una llamada más para obtener los datos del recurso es correcto

@ArnauPrat
Copy link
Author

Gracias @rickyah , estoy de acuerdo con tus observaciones.

@RdlP
Copy link
Contributor

RdlP commented Nov 11, 2013

Personalmente a mi, de las 3 propuestas que se han hecho, @adrigm @ArnauPrat y @rickyah la que más me convence es la de @rickyah puesto que creo que es la más sencilla y mas potente a la hora de extender su funcionalidad.

La de @adrigm le veía fallos a la hora de tener que añadir un nuevo recurso, que habría que modificar la clase y añadir métodos y la propuesta de @ArnauPrat le veo el fallo de tener un manager por cada tipo de recurso

@adrigm
Copy link
Contributor

adrigm commented Nov 11, 2013

@RdlP votas por la de @ArnauPrat o por la de @rickyah ?

A mi de las 3 (incluida la mía) la que más me convence por ahora es la de @rickyah, claro está con los consecuentes ampliaciones para manejo de rutas y demás.

@RdlP
Copy link
Contributor

RdlP commented Nov 11, 2013

Perdón, me he equivocado en el comentario, quería decir que la más simple y extensible me parece es la de @rickyah así que voto por ella.

P.D Voy a modificar mi comentario anterior para expresarme bien.

@ArnauPrat
Copy link
Author

La versón de @rickyah es objetivamente la mejor teniendo en cuenta las necesidades actuales, así que optaria por ella.

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

Esta noche iré mirando como mejorarlo, ya que como dije ese código esta hecho de cualquier manera y hay bastante que discutir, por ejemplo el hecho de que pidas un recurso con el tipo y te devuelva un puntero a ese tipo :P

@ArnauPrat
Copy link
Author

Si no lo he entendido mal, realmente ya estás devolviendo un puntero al recurso del tipo, GDE::ImageResource. Lo que pasa es que implementamos GDE::ImageResource usando sf::Texture, de ahi que tengamos que usar el getData, correcto? Si la implementación Texture fuera nuestra, el getData no haria falta, me equivoco?

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

Me refiero a que haces resourceManager->getResource<AResource>("id1"); y esa llamada en realidad devuelve un AResource* Quizá lo mejor sería devolver una referencia, que para devolver un puntero tengas que pedirle el puntero, (resourceManager->getResource<AResource*>("id1");)

Se podría discutir a ver qué es lo más claro.

@adrigm
Copy link
Contributor

adrigm commented Nov 11, 2013

Sí, dado que los objetos SFML reciben referencias no punteros. Sería mejor devolver referencias a menos que se pida un puntero.

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

Oído!
Aunque me temo que de momento dejaré que devuelva siempre una referencia, y cuando pueda desempolvo el Modern C++ design para saber cuando un tipo parametrizado es un puntero :P

@desclapez
Copy link

@rickyah mejor aún!

Que pensáis sobre los nombres de las funciones? No os parecen mejor sin la palabra resource en cada uno?

Quiero decir, en la clase ResourceManager veo bien que se le añadan, por ejemplo: registerResource
Pero en las clases Resource y sus subclases creo que estarían bien bonicas sin el nombre.

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

A mi me parece bien sin la palabra resource, pero entonces nombrar bien la variable de la instancia del manager:

Así si 👍

ResourceManager resourceManager = new ResourceManager();

resourceManager->get<ImageResource>()

Así no 👎

ResourceManager rm = new ResourceManager(); 

rm->get<ImageResource>()

@adrigm
Copy link
Contributor

adrigm commented Nov 11, 2013

Yo usaría más la palabra Asset que Resource. Resource los usan los sistemas para otros menesteres como iconos de la aplicación, manifiests... Además que Asset es más corto que Resource que puede parecer tonto, pero es una consideración.

En cuanto al uso reduandante de la palabra Asset/Resource estoy de acuerdo en que se puedo omitir. Yo usaria

assetManager->get();

@danigomez
Copy link
Contributor

Mmm, yo preferiria que si diga Resource, creo que le da legibilidad al código, y que solo diga "get()", se me hace corto, :P además la mayoria de lo IDEs tienen autocompletado por lo que no se gana tiempo de tipeo digamos :P,
Es decir, si se entiende qeu es lo que hace el método, no habría inconveniente en omitir Resource, por lo que es importante que usemos bien la nomenclatura de las variables y los tipos!!!

@DavidBM
Copy link
Contributor

DavidBM commented Nov 11, 2013

A parte de que forma sea más clara, os diré que, a menos desde mi punto de vista, me interesa que se haga uso de templates y formas de diseño avanzadas para aprender.

Usar clases de forma básica no es complicado, más bien es lo sencillo, lo realmente interesante y que cuesta es usar bien los templates y tener claras ciertas estructuras y su utilidad, por lo que veo muy interesante que cuando se haga el gestor de recursos, se explique por que de cada cosa y como funciona.

Soys unos cracks! :)

@rickyah
Copy link
Contributor

rickyah commented Nov 11, 2013

Ojo cuidao que los templates pueden ser un peligro, por un lado para entenderlos, que la metaprogramación con templates puede ser muy oscura. Y además pueden hacer subir el tiempo de compilación un huevo y la yema del otro.

Pero puestos a enfangarnos podemos empezar por aquí: http://loki-lib.sourceforge.net/

@danigomez
Copy link
Contributor

Hay una muy buena página para lo básico de C++03, y está en español!, http://c.conclase.net/curso/index.php, tiene ejemplos sencillos de practicamente todo el lenguaje, manejor de excepciones, templates, herencia etc etc.
PD: si es tremendo offtopic pero en fin jaaj

@danigomez
Copy link
Contributor

Qué pasa con esto al final??, se va a mergear o no?? Así vamos creando el módulo que corresponde según la nueva organización y seguimos avanzando con el proyecto... que parece que está muriendo un poco...

@adrigm
Copy link
Contributor

adrigm commented Dec 20, 2013

Mañana voy a poner al día un poco el proyecto y a organizarlo un poco mejor, no sé si @rickyah al final se ha puesto o no, si no lo ha hecho hay que ponerse ya para como bien dices avanzar. Necesitamos empezar a organizarnos mejor para que el proyecto no se pare cuando alguno no esté disponible como ha sido mi caso estas últimas semanas.

@rickyah
Copy link
Contributor

rickyah commented Dec 21, 2013

Yo también he estado un poco ocupado pero ahora tengo vacaciones.

Voy a mirar cómo hacer todo esto por que mi solución usa RTTI (dynamic_cast), y eso tampoco suele ser aceptable.

@danigomez
Copy link
Contributor

@rickyah Qué es lo qué estás tratando de solucionar?? Así me lo pongo a ver también :P y de paso me voy metiendo un poco más en el código del ResourceManager

@rickyah
Copy link
Contributor

rickyah commented Dec 22, 2013

A ver ni caso que no se ni donde vivo últimamente xD.

Cosas que hay que hacer: implementar el gist partiendo de master con la nueva organización del fichero.

Como no tengo nada hecho creare una rama en mi repo para ello y lo pondré aquí para que podáis mirarlo/clonarlo

Cuando lo tenga hecho hago un PR y lo revisamos

Lo que pasa que acabo de llegar hace 3 dias a mi tierra por vacaciones y claro, visitar a la familia por un lado, comilona por otro, chupito por otro lado y no da tiempo. Mañana empezaré a estar más liberado :)

@adrigm en otro orden de cosas, como eres editor de genbetadev, mírate gameinstitute.com
Una web con un curso de programación de juegos que tienen varios proyectos incluido un engine con fuente incluido. Lo que mola es que por oferta te dan todo el material de los cursos, pero no el curso en sí, por $49 hasta Navidad. Míratelo x si quieres ponerlo en la web, y si necesitas preguntar algo ponme un Mail que yo ya lo compré

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.

7 participants