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

Consider renaming App class #200

Open
nat42 opened this issue Oct 16, 2022 · 9 comments
Open

Consider renaming App class #200

nat42 opened this issue Oct 16, 2022 · 9 comments

Comments

@nat42
Copy link

nat42 commented Oct 16, 2022

Was browsing WebGL libraries and found this project, and a potential nitpic that jumps out at me is with the exported App class. I both suspect this could be an annoyance using the library in existing code which expects to have it's own App type and I don't feel the name seems appropriate.

To the extent that I understand the difference, Picogl.js is a library rather than a framework, and as such it should not be telling me what is or is not my "app" or how my project should be arranged, and the concept of what is part of any "app" objects imho should probably be left entirely to the consumer of the library to decide.

A year or so ago I was looking for a lightweight WebGL wrapper to do some rendering in a brownfields Angular SPA and a library wanting it's own App type would have been annoying (likely could rename the type in imports, hence choice to call this annoying rather than a potential deal breaker for someone).

Now if I felt the classname App was apt, I probably wouldn't raise this (I do understand this might be annoying to change considering it could break existing code if not handled with care like offering the export under two names for a period, with App marked as depreciated); but I feel that conceptually the "App" class does not represent anything like an app, and instead encapsulates the WebGL rendering context - a handle representing an instance of the state machine used by the underlying graphics library and methods that require such a context.

Because I feel the name is incorrect in describing what it is, and has a potential to atleast annoy consumers by conflicting with existing code (and what they think of as being part of the "app" when they work with the library) I feel I should suggest considering the effort to rename the class. I hope I don't sound too obnoxious describing this, and leave it up to your judgement whether this warrants considerations. Thanks.

@lucasdomanico
Copy link

Can you suggest alternatives names?

@nat42
Copy link
Author

nat42 commented Oct 29, 2022

I'd probably call it like PicoContext (the main instance could be abbrebriated to like ctx) so you'd have like:

let app = PicoGL.createApp(canvas)

become

let ctx = PicoGL.createContext(canvas)

but that betrays perhaps that I see the library as wanting to be a lightweight OOP wrapper around WebGL for coders somewhat familiar with WebGL or OpenGL APIs.

Other ideas I can think of might be like Renderer, PicoRenderer, PicoGL or PicoMain; I wouldn't use them, but they wouldn't have the issue/s of "App".

@lucasdomanico
Copy link

I don't understand the problem. Is it just the name of createApp()?
If the problem is the Typescript type, you can easily use an alias like type WebGLState = PicoGL.App

@movAX13h
Copy link

movAX13h commented Jun 26, 2023

I agree with the original poster.
In my project, I'm using PicoGL in several small parts of an application so it feels weird to createApp() when I'm actually just assigning a canvas to be used with WebGL.

Edit: Yes, for me it's just the name of the function createApp ... very minor issue :)

Edit 2: My suggestions are:

  • PicoGL.picofy(canvas)
  • PicoGL.activate(canvas)
  • PicoGL.init(canvas)

@lucasdomanico
Copy link

why not just; let Pico_init = Pico.createApp?

@movAX13h
Copy link

There are 3 options:

  1. You are trolling
  2. You don't understand the point of a public library
  3. You don't know why we put code on github

Maybe it's all 3 of them, not sure.

@lucasdomanico
Copy link

Rather than attacking me, why don't you explain what's the problem with creating a wrapper?

@movAX13h
Copy link

I'm quite sure that everyone here knows how to create wrappers or aliases - but that's not considered a solution - it's a hack. Thus option 1.

Regarding option 2: If someone is using this library via npm, you cannot expect them to modify the source code of the installed package to add an alias or wrapper. That would be counterproductive since it would revert on update or if someone else is taking over their project and runs npm install, the project will not compile because of missing changes. As mentioned above, adding an alias or wrapper to the user project code is very messy. You dont want to do that for all 50 libs you might use in a project and it's also not the point of having a library.

Issues on github are there to make progress on the library, not to suggest short-time workarounds. It's about the codebase. That's what led me to consider option 3.

The fact that this issue exists that has a very thorough original post indicates the need for change or at least a discussion about it. You're not helping anyone with workarounds so that in the end everyone has a different variant and no progress on this library has been made. You're just telling us to hack it in on our side - thats quite ignorant and discouraging.

Anyways, no hard feelings on my side. I'm just trying to solve problems.

I'm wondering if @tsherif is still around ... there are some pull requests waiting to be processed. I've been using this library for several projects and will continue doing so. Preferably I'd like to use this (original) repo/npm package and not a private fork.

@lucasdomanico
Copy link

lucasdomanico commented Jun 26, 2023

I see your point. Are you aware that the only interface of PicoGL is createApp? https://tsherif.github.io/picogl.js/docs/PicoGL.html
I know that this add a layer of complexity more. But I would create a module WebGL2State.js (or whatever), like:

import PicoGL from './PicoGL.js'
export default (...args) => PicoGL.createApp(...args)

And then just it like;

import WebGL2State from './WebGL2State.js'
let state_1 = WebGL2State(canvas_1)
let state_2 = WebGL2State(canvas_2)

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

No branches or pull requests

3 participants