libglui / glui

GLUI is a GLUT-based C++ user interface library which provides controls such as buttons, checkboxes, radio buttons, and spinners to OpenGL applications. It is window-system independent, using GLUT or FreeGLUT.
Other
200 stars 81 forks source link

Heads up! I'm rewriting GLUI right now/will pass along the code shortly (Widgets 95) #91

Closed m-7761 closed 4 years ago

m-7761 commented 5 years ago

Way back in 2017 I was kind of active here, testing the waters. I'm now doing the rewrite I said I would do. First I'm doing a transformation that retains all of the non-deprecated code. When I'm done I will upload this somehow. If anyone wants to use it to make a new branch for GLUI.

I'm doing this ahead of using the code for a new project/library at https://sourceforge.net/projects/widgets-95/ that will drop GLUT in favor of wxWidgets. It's going to be called "Widgets 95" that is a play on Windows 95, since that's what GLUI looks like. It will not be linked to GLUI other than it being a derivative work.

Before switching to wxWidgets the code will use all of GLUI's names for things. That is the code I will provide as the basis for a new version of GLUI. The refactored code uses a namespace, and drops the GLUI_X prefix. The control-based classes, I believe, will be nested classes, nested inside the GLUI class, that I've renamed UI for the rewrite. That makes it clear which classes are windows/controls, because they all use the UI:: scope-resolution. It makes them friends of "UI" and in my vision, users would derive their own dialog from the UI class (I've done this myself) and so within that class's scope, they can use the names of the nested classes without the UI:: decoration.

I'm just explaining what will be different, structurally. Deprecated/unused members/methods will be removed. Code will be refactored to be highly legible. Tabs will be used, but can be undone if not desired. Private members/methods will use _ to prefix them, so it doesn't look like a public member/method. Some methods will probably be made inline. Classes will not be exported, just their methods will be (exporting a class exports all of their methods. Better to do this selectively.)

I'm moving the various enum like values from being macros to const/static values, nested in their relevant context. Whoever reviews the code can set up compatibility typedef and constants in the global namespace if you see fit to. If the code is not accepted, that's fine with me. It's just a lost opportunity for GLUI is all. I want to contribute something, a gesture of my gratitude, for GLUI.

I think GLUT's days may be numbered. But it can't hurt to spruce up a project like GLUI even still, from time to time. The new code will be a little out-of-sync with the current code, but it should be simple to look through the recent changes here and reapply them, I think. There were just a few. I will do this myself if my code will be accepted. I may have this finished as soon as in a few days time. Next week at the latest.

nigels-com commented 5 years ago

Best wishes for your venture. Have fun in innovation mode.

m-7761 commented 5 years ago

Not innovating, just sanitizing. For my own sanity. Having legible code makes easier to be more correct, and to innovate, and makes it more attractive. After there is a base, I will really go to town on it, but at that stage it won't be GLUI at all, unfortunately. I will try to fix some of the most egregious parts, if only because it will make the sanitization process easier to complete.

Here (attached) is an early version of the header. I got derailed tonight on the TreePanel code. It really threw me for a complete loop. I guess you get what you ask for. It will be like this, except for a few changes. The exported methods need decoration (I will have to decide which are appropriate.)

The strings should be replaced with pointers/wrappers if not a new custom type. For end-user convenience C++ references could be used instead. That std::vector uses a fixed size anyway, and so can be a fixed size C array. That would solve all the library issues.

It's a headache to rewrite, but it's a straightforward process. People overestimate the work involved in developing something from nothing. If something works, there's value in it. Even if it's not particularly well organized, or very sloppy. I renamed the headers to not confuse them in the editor. "xcv" is code for `95 (using roman numerals) that just happen to be the cut-copy-paste sequence on a querty keyboard. I don't know if that's auspicious or what. Anyway, the file is glui.h's. It has to be zip for GitHub to attach.

xcv.zip

m-7761 commented 5 years ago

Uhm, I'm going to post any obvious bugs I stumble across in this Issue, so not to make more issues.

For starters, these don't seem to be initialized. If they get zero-initialized that's okay.

    void (*glut_mouse_CB)(int, int, int, int);
    void (*glut_keyboard_CB)(unsigned char, int, int);
    void (*glut_special_CB)(int, int, int);
    void (*glut_reshape_CB)(int, int);

Otherwise, they appear to be called by these:

void glui_parent_window_reshape_func( int w, int h );
void glui_parent_window_keyboard_func(unsigned char key, int x, int y);
void glui_parent_window_mouse_func(int, int, int, int );
void glui_parent_window_special_func(int key, int x, int y);

Which don't seem to be used internally, but are advertised. Maybe someone knows if it's okay to rip these out? I can't tell just yet.

m-7761 commented 5 years ago

glui.cpp has an uncommented on "AND 0" in a funny place. It's disabling this code, but maybe it was left in by accident by someone doing a quick test?

      if ( active_control AND
           active_control->active_type == GLUI_CONTROL_ACTIVE_MOUSEDOWN AND 0)
      {
        /*** This is a control that needs to be deactivated when the
        mouse button is released ****/
        deactivate_current_control();
      }
m-7761 commented 5 years ago

Anyone know what glui_window.cpp (empty) and viewmodel.h and viewmodel.cpp are about? Are these internal? Or public utilities?

Keeping dead code around is bad code hygiene. I think it's misguided to keep junk code because past clients may (theoretically) reference it. If those clients link against an unversioned version of a library, then they should break if unmaintained.

If GLUI is not to languish (completely) it needs to at a minimum wrap its junk code in preprocessor directives. Better to opt in than out. Deprecated code should not compile after a few release cycles, or a few years.

m-7761 commented 5 years ago

I'm assuming projects don't use GLUI_Bitmap. It only has initialization for grayscale images. I'm going to assume those are just the images in glui_bitmaps.cpp, etc. I'm going to not copy those, and not have a copy of them all also copied for every single GLUI. I worry if people see this they will go mad or something, like staring into the face of some cosmic horror.

m-7761 commented 5 years ago

Tree::get_id replaces Control::get_id which is a virtual method. However the original uses const and Tree does not, therefor it will not override. On the other hand, set_id also exists, and Control::set_id is virtual too, however it does not use const because I guess, it's doing assignment.

It's hard to tell if const is being used here to foil virtual method inheritance, or if it's just used by a few methods, and not at all consistently if so.

Nothing else overrides these virtual methods. It seems like Tree could just use the existing ID member, but I haven't looked into it. Tree/TreePanel is kind of a mess. More so (https://github.com/libglui/glui/issues/93) than the usual.

Not really related, I'm introducing a template to implement the virtual methods. There's two reasons it's needed. One is the editable text element overloads set_text to work with the String class. That overload should logically be done in Control::set_text so that all controls have a standard overload. However, that means defining the virtual override will hide the overload methods. So they must be unhidden some how, kind of defeating the point...

The second reason is GCC has never implemented a feature to export virtual methods. That means it can only export by wrapping a non-virtual method inside a virtual method. It's an annoyance, but it solves the overload problem to to use a standard template to generate the virtual method tables. What exporting the methods buys you, is the classes can be user extended. And if you ever want a sane library system, there has to be a way to instantiate the classes on the client's side. By generating the virtual table there, the client can fill out a method that can be used to safely delete the elements when the UI is torn down.

Designing a useful library with C++ is actually something that only veteran developers who understand the ins-and-outs of its quirky memory model should undertake. For users, it can be time-consuming nightmare to mix and match badly written libraries.

The String class should also be changed, in order to be fully accommodating. If it cannot be its own class, then it should be a class with some virtual methods that inherits from std::string. If the library is to generate elements internally, i.e. with new then it needs a client supplied factory for allocating/deallocating string pointers. By using pointers or C++ references, the strings have a known ABI. And so it's safe to use them via their virtual methods.

EDITED: It looks like this:

class UI::Listbox : public Interface<Listbox,Control>
{
public: //...
}
class UI::Rotation : public Interaction<Rotation,Mouse_Interaction>
{
public: //...
}

Interaction is just another interface. I called it interaction because Mouse_Interaction needs it. I don't think any classes have other virtual methods. If they do, they may need to be added to the Control list.

nigels-com commented 5 years ago

One suggestion is to go header-only. I'm not sure there needs to be global variables or dependencies hidden-away in a compilation unit.

m-7761 commented 5 years ago

There is a memory link in GLUI_Node::unlink because it zeroes its children pointers. It's not used on controls (confusingly) but is used on list items and the GLUI main window objects. But deletes the "panel" control. List items are flat. But Tree items are not. The TreePanel method looks like it leaks.

Logically "unlinking" seems like the children should be retained. I've changed it. I had restricted its access. But it's used pretty broadly. I suppose I will make it public, but still underscored as to not advertise it in the Node derived classes.

m-7761 commented 5 years ago

Sorry, I didn't catch your reply:

One suggestion is to go header-only. I'm not sure there needs to be global variables or dependencies hidden-away in a compilation unit.

Header-only makes sense only for extremely streamlined problem domains, or where there are significant compile-time benefits to be had. GLUI is a classic example of where polymorphism makes the most sense.

Header-only bogs down large project build-times. And it can't be patched or even rebuilt without rebuilding translation units that depend on it. I'm not saying it's the wrong idea. But it puts a burden on consumers in many cases. I think it would be wiser to just develop an internal String class if the goal is to get around std container issues. The real problem with std containers in my opinion is that they don't play well with anything except themselves. So if you don't want to manually translate between them, you're forced to inherit from them. That's really wrong. I mean, at a minimum it should be possible to assign a range to them. There's obviously an agenda to avoid that possibility. This makes people feel like they have no choice but to use std::string, etc.

The only reason to use templates is this case, is GCC. But it's useful for overloading methods too.

The only global variable I've seen so far is the OpenGL texture ID used by the Rotation control's texture. It's a problem for multi-context OpenGL, or if the context is destroyed. I mean, there are others, but they are appropriate.

nigels-com commented 5 years ago

You'd be amazed how much of the maintenance I do for GLEW is related to build issues, and link issues in particular. And that's for a single compilation unit of ANSI-C that could usually be encapsulated as a single invocation of gcc.

So when I hear things such as "templating for gcc" or "avoid using std::string" and "windows runtime library quirks" it does make me wonder about optimising for an easy as possible on-ramp for new users, rather than intricate ABI sort of compatibility and maintenance.

m-7761 commented 5 years ago

I forget you work on GLEW. Do you have any thoughts on what would make a good namespace for GLUI? Its name is rather short. I prefer specific namespaces when possible. Libraries written in C++ should always use namespaces. The only excuse for why there are so many that do not is those are C libraries. C++ libraries like GLUI can also encapsulate C libraries so that they don't pull C globals into the global namespace. (I frankly don't understand how anyone can work with C. Software development is so slow and difficult, and C just compounds it.)

I think with GLUI it's not even correct to assume its consumers use OpenGL directly, although they must probably interact with GLUT. Like I've said before, it would be easy to wrap GLUT that I think that is a service that GLUI could benefit a lot from. Maybe with the use of a namespace and rewritten things like this will seem more worthwhile to someone.

The templates make for less code. I will use the same format for my Widgets 95. It's going to encapsulate wxWidgets and other modes, including 2D and 3D rendering. My intuition is trying to make GLUI headers-only is a bad idea. It's just intuition, but consider that header-only only exists because C++ templates are header-only beasts. It's actually a big problem for C++ and not a benefit. Libraries that can operate with some restraint should. Even if you think GLUI is only suited for small, toy apps, I don't think you should, since 3D apps often wrap around many different libraries, because doing anything in 3D is a lot of code, even if it's being pulled off the shelf.

I don't have any dog in GLUI really, after I'm done with prepping it. If it was header-only it would be stuck pulling in GLUT and OpenGL forever, and so it would never have an off-ramp to migrate away from those, or offer more options. The Widgets 95 code I develop could be back-ported to GLUI. I don't know how professional wxWidgets is all around, but the menu for tools like GLUI is pretty barren. And even things like GLFW and GLUT I feel are too restrictive, don't offer significant features for modern software. I think it's a space ripe for innovation. wxWidgets seems like a better path forward for its cross-platform feature set.

m-7761 commented 5 years ago

DOUBLE-POST: Just to reiterate. Function overloading is actually one of the best parts of C++. If you aimed to modernize GLUI it's a good first step to figure out an overloading framework, and templates can help a whole lot to avoid rewriting code, or using macros.

Having a template hook into every class will open up space for a lot of innovation. And it will make standardization possible. Overloading methods can give users unlimited flexibility. It can shorten names of methods too, so that you don't have specify types and so on in the method's names.

m-7761 commented 5 years ago

GLUI_Button button; appears in the Rollout and Tree classes. This large data member is not used by any internal code. If it were linked into the node graph it would crash, etc. if deleted.

It would be nice to mark embedded controls so that they are not deleted/destructed. I've streamlined a lot, by having Node::~Node "unlink" from its parent and then set each child's parent to NULL before deleting it. The controls all have String and Node members that need to be destructed. By doing it automatically it's unnecessary to implement destructors. I've added a virtual method Node::_delete that matches delete to its originating new so this process is safe from whichever C++ Runtime the elements originate.

The downside of this is constructors need to use inline so the virtual method table is generated where new is called.

I think the best strategy I can come for String is to use a C++ reference, and add a pointer to the class, so that when the String holding the control's name string is destructed it will set off a chain reaction deleting any additional strings, so it's not necessary to use destructors just for strings. But a simpler way would be to just use "getter" methods to get the underlying reference. Then the strings could just be wrappers that automatically destruct themselves.

If it were me, I would just do a breaking-change and make all data members private, and put _ in front of their names, because that then makes it very easy to see what are members inside method definitions. In general I think GLUI would benefit from making a lot more of its members/methods protected. Maybe most of them seem to only be used to implement the UI. Even the interfaces seem mostly internal to me. I think it's unclear to the user/consumer what methods are for them to use and vice versa.

nigels-com commented 5 years ago

consider that header-only only exists because C++ templates are header-only beasts

I was pointing out that link libraries bring their own set of limitations, complexities and problems. The nature of GLEW is that it can't be header-only. But it seem plausible for GLUI, it's not all that much code, and it need not all be included from everywhere.

I'm in the process of refactoring some old objected oriented C++ into a modernised more narrowly-focused set of tools. Making everything inline and single-source-file has some nice side-effects such as a much simplified build scheme.

For GLUI I'd suggest the namespace glui::, off the top of my head.

-- Nigel

m-7761 commented 5 years ago

glui_list.cpp has the following code, without // that I recommended removing I think in a past PR. The only difference now, is I noticed other timing code uses GLUI_Time. It seems like this code should too, unless there is a case not too. But I recommended against using it, because double-clicking timeouts don't work on X servers or remote systems that have to ping back/forth versus queued events. In my experience double-clicking is unnecessary for selection based elements, and it can be hard to pull off if you're injured or your mouse is on its last leg. (I buy premium mice, so try to stretch them as far as they can go.)

Reason being is if something is clicked one time, and so selected, there's no need to click it again, except to double-click. So there is no need for a timeout. I personally had a bad local X server experience with this code. But it might work better with GLUI_Time if it is smarter about how it generates its time, since it's designed to work with an X server. I don't know what ftime is for, but it's probably not for X events.

    //unsigned long int ms;
    //timeb time;
    //ftime(&time);
    //ms = time.millitm + (time.time)*1000;

On namespaces, I wonder if something like GLUI_2 would be better. I think it would pique future user's interest, since they would naturally understand that it's an addition. I don't think namespaces should try to be short acronyms. I think they should be as specific as possible. But I don't know how to make "GLUI" more specific because it's already an acronym itself. It shouldn't be super long for debugger visualizers. But C++ has namespace glui = GLUI_2; and that way users can decide if they prefer lowercase or what. Acronyms are technically capitalized. I just wonder if my choice to use UI (which doesn't have to be kept) would look funky with glui::UI::Control or if lowercase names should be used or defined. There are a lot of options, but namespace glui = GLUI::UI is another. Another reason not to use short/cryptic acronyms is you are actually taking that name out of the global namespace, and so are more likely to collide or have a conflict. That's why specificity is important in choosing a namespace, mainly for linking purposes.

Namespace choices can be important. Like earlier today I stumbled across the glbinding project again. That looks attractive, though a bit overkill. But I've never bothered with it because it uses gl for a namespace, and then uses gl again for a prefix in that namespace. So I just don't like that part, and so decide I would rather just eek out with vanilla OpenGL. Small decisions like that can make all the difference. I think if I ever choose an OpenGL wrapper (I'm really wondering these days if it's better to move onto Vulcan or something) it will be an OpenGLES 2 wrapper for desktops. It annoys the hell out of me that things like OpenGLES and OpenVG never get desktop implementations. I don't know how healthy OpenVG is, but it has lots of phone implementations. We could use vector-graphics on desktops too! It's so weird. (EDITED: I don't want to rag on glbinding. For all I know it's configurable/header-only. I just wanted to make an example of what to look for in a namespace.)

nigels-com commented 5 years ago

I tend to expect to see namespaces in lower case. std and boost being the most common.

m-7761 commented 5 years ago

Scrollbar::associated_object is a void* but Scrollbar::update_size assumes it is a control pointer. update_size is an interface that is used. So it seems like the member should be a Control* type.

I tend to expect to see namespaces in lower case. std and boost being the most common.

Well, conventional wisdom in the programming world is always in flux (proving there is little wisdom to go around) but gradually the guidelines (https://github.com/isocpp/CppCoreGuidelines) project has caught up with my own thinking about 80% of the way there. It remains internally inconsistent in many ways. And it leans too far toward linear thinking programmers. who are unsuited for nontrivial programming tasks. I give it 20 or 30 more years to start marking more sense :)

I think rules are meant to be broken. Like if every library uses the same style guidelines, it becomes difficult to tell what is what, where one library space begins and ends. Rule of thumb for not using camel-case, and using underscores to separate words, but only to delineate semantics when so, is to capitalize where you would do so when writing in plain English. That means proper names are capitalized, and acronyms are capitalized. In other words, there is no special meaning given to capitalization.

BOOST would probably be better for that project, but it sees itself as so important to displace every other project, permanently seating itself on that identifier. I think it's common courtesy for more obscure projects to stake out longer, more specific names for their namespaces. But since when have human beings ever done things courteously or sanely right? I'm old fashioned, so I don't like it when writing is not capitalized. It looks confusing and lazy. I would use glui probably myself mind you. But that's an alias, and not a namespace. There is a big difference. But also, it can depend a lot on the situation, which should be in the control of the user. Like if the user already has a bunch of punchy lowercase namespaces peppering their code, they might prefer to use GLUI to make it stand out. So, ultimately, it doesn't matter. Even LibGLUI or something can make more sense, to make it stand out. I'm just being conversational. Users may ever do GL::UI::Control or something. You never know.

Correction: "namespace glui = GLUI::UI" would have to be typedef GLUI::UI glui since UI is a class. I think that would be viable, since the things outside of that class are accessories mainly. Truthfully, the way I imagine it, is instead of using the namespace itself, I'd probably do class UI : public GLUI::UI and start from there to build my UI. In that case the namespace hardly matters at all!

nigels-com commented 5 years ago

Since GLUI is already version 2, I'd suggest another name for an incompatible derivative. To minimise broader confusion.

m-7761 commented 5 years ago

Since GLUI is already version 2, I'd suggest another name for an incompatible derivative. To minimise broader confusion.

The reason I came up with 2 is not based on version, but to give an idea that there is a compatibility layer in glui.h. How I would set it up is not incompatible, nor derivative (since I have no interest there) but to have a new header (I explained this in another, older Issue the other day) that would supplant glui.h (2) by having glui.h include the new header, in addition to defining a compatibility layer, so that code that includes glui.h will get the compatibility header...

And so in that way, if I'm looking at GLUI for my project, I will have a choice of including the new or the old, and being a new project, I'd include the new, so that the unnecessary stuff is not included in my project. Old code remains none the wiser.

It's impractical to keep every single thing intact (which is always the case for living code) but the difficult parts are pathways that most projects would unlikely touch.

If my work is not integrated into this code, I will probably make a fresh GitHub project for it, without any revision history, since GitHub doesn't seem to provide a way to fork projects under a new title. I think that makes the sources of forks look more authoritative than they have a right to be. There will be a fork also. I'm thinking it will just take 30mins to set all of that up when I'm done, for posterity. Also, that will give users a way to migrate to a new, living version of GLUI and fork that.

nigels-com commented 5 years ago

It's possible to rename a github project. As an example: https://github.com/nigels-com/electron-leaflet

I'm not sure why you'd want to collapse the revision history, or obscure the authorship and license context.

m-7761 commented 5 years ago

I wouldn't. But GitHub has its Fork system. And I didn't know that it is possible to rename/list a fork independent of its source. And I suppose there is some way to import a git tree into GitHub, but that idea had not occurred to me. I still think it's probably not necessary to retain the revision history. But if that's important to somebody... I'm glad you seeded the idea. I think I would've thought of it though, come time to upload. (EDITED: I see how your link has renamed a fork.)

m-7761 commented 5 years ago

GLUI_DRAWINGSENTINAL_IDIOM/GLUI_DrawingSentinal are just doing busy work. I assumed they were managing a transform (translation) stack or something, but they are just repeatedly swapping OpenGL out of frontbuffer/backbuffer mode, backing up its state, and restoring it, to no purpose.

It's also switching the GLUT window every time a control is drawn.

This logic only makes sense in the glutDisplayFunc callback. I was moving the structure into the main header so users could use it to draw their custom controls. Anyway, I'm going to rip it out. And just put its code in the main window display function. The controls cannot even draw themselves at their own location without draw_recursive. Drawing seems like it must be an internal operation to me.

m-7761 commented 5 years ago

Note: The following Control methods don't make sense unless controls are drawn individually:

        //Don't these make more sense for UI?
        int set_to_glut_window();    
        void restore_window(int orig);
        void redraw_window(); /** Redraw everybody in our window. */    
        /** Redraw this control.
        In single-buffering mode (drawing to GL_FRONT), this is just 
        a call to translate_and_draw_front (after a can_draw() check).
        In double-buffering mode (drawing to GL_BACK), this queues up 
        a redraw and returns false, since you shouldn't draw yet.
        */
        void redraw();    
        void draw_recursive();  
        void translate_and_draw_front();
        void translate_to_origin();

The first 2 are arguably useful, but would make more sense to access them through the top-level container, since they are just forwarding the call to it. The rest are all drawing related.

The member that controls the buffer mode is private so it must not be changed by any extant software packages. Therefor, these methods don't make any sense to keep. Or are only useful for internal calls by the display manager at best.

The buffer mode was for choosing between single buffer and double buffer. But the drawing method doesn't really make sense, unless it was for computers that cannot draw the entire screen at an acceptable frame-rate, since if you are doing single buffer draws, they should timed to the display's vertical-blank period, and the entire UI complex should be rendered in that space, so there are not overdraw artifacts. Therefor all of this code looks archaic.

EDITED: Adding the following code to the glutDisplayFunc callback seems like the reasonable solution using GLUT. Otherwise I've changed the GLUI_DRAWINGSENTINAL_IDIOM to a "no op" so its code can be reviewed.

    //2019: Supplementing GLUI_DRAWINGSENTINAL_IDIOM
    _buffer_mode = glutGet(GLUT_WINDOW_DOUBLEBUFFER)?_buffer_back:_buffer_front;
m-7761 commented 5 years ago

FWIW I tried different namespaces. I think I prefer GLUI_Library as a namespace. It looks like GLUI_Master. So it fits into the existing scheme. The original manual (PDF) calls it GLUI Library (GLUT-Based ... Library) and I feel like this is verbose enough to not repeat the same problem C++ namespaces were intended to solve (name collisions in the global namespace ... which unfortunately requires plunking a name down into the global namespace!)

My main concern with others is they look cryptic to me, except for something like libglui maybe, or libGLUI, but I don't like names that end with UI because "GLUI::UI" just feels wrong. I prefer intelligible namespaces to random strings of letters.

m-7761 commented 5 years ago

GLUI_Control::set_to_glut_window() returns 1 on failure. I'm pretty sure it's supposed to return -1.

m-7761 commented 5 years ago

Tree and TreePanel don't use one set of their two colors. TreePanel doesn't initialize the color that it does use. It does initialize the color that it does not use. I'm more and more convinced its code has not been used, other than the example. I think ALTERNATE_COLOR doesn't do anything as a result. I recommend removing Tree/TreePanel from Example6.cpp until it can get serious work done, or not. It just makes the example freeze, and it's total crap honestly, since none of the buttons seem to work or have any real effect. I think it was premature to publish it, or its code rotted.

m-7761 commented 5 years ago

Listbox:;delete_item has logic to recalculate the width/refresh that is performed only when the item is NOT deleted. Needless to say, it follows there is no refresh when the item IS deleted.

m-7761 commented 5 years ago

I switched to using GLUI_Library as an afterthought today. I think the library is really shaping up. I've finished the top to bottom rewrite, and now just have to work on the constructor pathway. And the live-variable stuff needs to be made modular. It needs to be able to support double so templates (with virtual methods) will work very well for it. I think probably any floating-point functions should use double also so that the precision of the internal operations don't get truncated to float values.

After this work, I just have some landmines I've set for myself to go back over. I think an interesting perspective, is using GLUI_Library that is also in acronym form GL, so that a good namespace approach is namespace gl = GLUI_Library; or namespace gl{ using namespace GLUI_Library; } and so it can drop into a generic GL namespace. Then you get gl::UI as the top-level window class. And gl::GLUI is the new GLUI_Master replacement. But I've broken it up today into GLUT and GLUI but because of historical reasons, the GLUT class inherits the GLUI class, so that its methods leak into it. But users can use the appropriate one. I also set up a singleton pattern so that :: can be used with their methods to better illustrate that they are singleton objects in code.

I think it could evolve into a cool OpenGL centric project. If it could somehow integrate some minimal OpenGL stuff. To provide rendering support. It can encapsulate a lot. It can easily fully encapsulate GLUT. Maybe it could transition to GLFW via GLUT and later provide a switch over to its SDL option.

Now that it is well organized it's easy to see what it does. It's pretty simple at its core. I think it should remain simple. And maybe I will keep up with it, I don't know. I might backport some work to it over time. I'm going to do some experimenting with it. I think if it had a second round of work, after I finish my Widgets 95 draft, that makes them overlap as much as possible, it would be easier to maintain both.

m-7761 commented 5 years ago
    /* These variables store the last value that live variable was known to have. */
    int             last_live_int;  
    float           last_live_float;
    GLUI_String     last_live_text;
    float           last_live_float_array[GLUI_DEF_MAX_ARRAY];

These variables don't seem to be actually used in any appreciable way. Maybe they used to be compared in order to fire a callback, however now they are just kept equal, and the comparison that is done is done to the user provided pointer.... however, the only difference it makes is whether the control is drawn or not.

And this only happens through manual intervention of the sync_live method. I don't believe I'm missing anything.

m-7761 commented 5 years ago

I've since come across some code that uses comparison logic on previous values. The controls tend to store these values in their extended members. Which explains why last_live_int and friends are not used. I don't know why libraries don't remove dead members promptly. I see this a lot. I don't know if contributors are afraid to, or if there is an unhealthy obsession with binary compatibility mixed with a laziness to never do any housecleaning. I wonder how much software has data structures that are 50% unused data. In GLUI_Control's case it's probably more like 95% unused data. I've trimmed the structure's size down progressively so that it's more like a regular data structure's footprint than an eyepopping memory devouring curiosity.

I have had limited time lately. I've worked on constructors and live-variables, which are kind of the same problem, since the constructor parameters invariably involve the live-variables.

Below is a sample, including a fix for the CommandLine class's buffer which probably would've thrashed memory pretty hard prior to "move-semantics" adoption by C++ containers. I'm unhappy with the constructors but, obviously, I can't come up with a better system. I don't like it that user-controls would need to use macros like this. I mean obviously they don't, but it's designed to make things manageable.

I think I'm going to add a common callback to the window like I discussed in 2017. The example code just passes the same callback control_cb to every control, which is just a lot of typing. I'm ashamed that I was met with resistance for suggesting this then. I'm ashamed a lot these days. For my race. (EDITED: The human race.) With a standard constructor template (macro system) it would be more straightforward to add some kind of callback convenience feature. Like making it possible to pass a different kind of callback in place of an ID. I mean, if you are doing one callback per control, then an ID is probably not even required in most instances.

/************************************************************/
/*                                                          */
/*               CommandLine class                          */
/*                                                          */
/************************************************************/

class UI::CommandLine : public Interface<CommandLine,EditText>
{   
public: //Interface methods

    LINKAGE void _activate(int how);
    LINKAGE bool _key_handler(unsigned char key, int modifiers);
    LINKAGE bool _special_handler(int key, int modifiers);

public:

    #ifdef GLUI_COMMANDLINE_HIST_SIZE
    enum{ HIST_SIZE = GLUI_COMMANDLINE_HIST_SIZE }; 
    #else
    enum{ HIST_SIZE = 64 }; //100
    #endif

    //2019: The way this was implemented is pretty unintuitive
    //and dangerous. oldest_hist is basically the 0th index in
    //the vector, but it continues to go upward. So you cannot
    //access an index less than it. After it exceeds HIST_SIZE
    //add_to_history does erase(begin()); push_back(""); Since
    //move-semantics were added to C++ containers this is only
    //shifting the entire vector. Prior it would've been a bad
    //memory stategy to say the least. I've tried to implement
    //it as a circular buffer to not have to shift. I think if
    //it had a Listbox feature it could work as a location-bar.

    String hist_list[HIST_SIZE];
    int curr_hist;
    int oldest_hist; 
    int newest_hist;
    bool commit_flag; 

    //2019: add_to_history uses this size.
    const short _add_to_history_hist_size; 
    int oldest_hist_wrap; 

public:

    inline const char *get_history(int command_number)
    {
        //2019: This code demonstrates the earlier methodology.
        //return hist_list[command_number-oldest_hist].c_str();
        return get_history_str(command_number).c_str(); 
    }
    inline String &get_history_str(int command_number)
    {                               
        //Note: This makes this a little saner for users
        //to interact with since it can't go out of bounds.
        int i = command_number-oldest_hist+oldest_hist_wrap;
        return hist_list[(unsigned int)i%HIST_SIZE];        
    }

    LINKAGE bool add_to_history(const char *text);
    inline void reset_history(){ _members_init() }
    inline bool scroll_history(int direction)
    {
        return recall_history(curr_hist+direction);
    }
    LINKAGE bool recall_history(int history_number);    

    void dump(FILE *out, const char *text);

    template<class LV>
    inline CommandLine(GLUI_ARGS(,LV *live_var,)){ GLUI_LIVE }  
    inline CommandLine(GLUI_ARGS(,)){ GLUI_INIT }
    inline CommandLine(){ _members_init(); }
    inline void _members_init()
    {
        //REMINDER: This implements reset_history.
        curr_hist   = 0;
        oldest_hist = 0;
        newest_hist = 0;
        commit_flag = false;

        //2019
        //hist_list.resize(HIST_SIZE); 
        oldest_hist_wrap = 0; 
        (short&)_add_to_history_hist_size = HIST_SIZE;
        assert(_add_to_history_hist_size==HIST_SIZE);
    }
};
m-7761 commented 5 years ago

RadioGroup seems to draw its buttons twice. It has two different drawing paths. One looks older. I suppose it's that way so that it draws its buttons with both draw and draw_recursive but in the latter case it draws them and them each button is drawn again, recursively.

I'm removing the old routines. There's an unused draw routine that it's unclear what it's for. It's drawing points, so maybe it's a keyboard navigation indicator? But it's not used. It's not how Windows highlights. EDITED: There's also a (used) method for drawing the highlight called draw_active_area. I'm removing the old code. The draw code is not exposed, so that the only way to draw is from inside draw_recursive and I think that I've made that a private method if it was not already so.

void UI::RadioButton::draw_O()
{
    GLUI_DRAWINGSENTINAL_IDIOM

    glBegin(GL_POINTS);
    for(int i=3;i<=UI_RADIOBUTTON_SIZE-3;i++)
    for(int j=3;j<=UI_RADIOBUTTON_SIZE-3;j++)
    glVertex2i(i,j);
    glEnd();
}
m-7761 commented 5 years ago

The update_size interfaces don't trigger repack. This means (unless I'm missing something) users must call GLUI::refresh after resizing. I've removed some code that preemptively refreshes the entire layout every time a control is added, in favor of setting a flag to pack the layout on display or when the size is queried. But I haven't extended the flag processing to the control dimensions themselves. This could break some apps if they expect the controls to be positioned immediately after adding them. But it seems somehow appropriate to not do major computations needlessly. The same courtesy is not extended to operations that change the dimensions of controls.

To do that, I've added a resize method to complement the redraw method. The post_repack_main_gfx method is also added. And something I meant to share in earlier posts is I've added a callback to be able to draw over the UI before it flushes/swaps its buffers. Calling resize doesn't repack. That's done with refresh I guess.

    /* 2019: update_size doesn't trigger repack. */
    inline void resize(bool now=true) 
    {
        if(now) update_size();
        if(can_draw()) ui->post_repack_main_gfx(); 
    }
m-7761 commented 5 years ago

GLUI_Rotation doesn't have a destructor. These objects are leaked:

    Arcball        *ball;
    GLUquadricObj *quadObj;
m-7761 commented 5 years ago

hide_internal (a very counterintuitive API) works on siblings. The second phase that works on the collapsed nodes don't however. That is performed on the children (of this) instead of its siblings.

I almost missed this. I thought it was just a recursive hiding operation. I was removing the collapsed_node field when I noticed it. (Instead of a bool and a Node object, it can be a pointer to a Node in the controls that are actually collapsible.)

m-7761 commented 5 years ago

~GLUI_Rollout doesn't have a destructor. If it's collapsed its nodes are leaked.

EDITED: Of course ~GLUI_Control doesn't delete them either. I think the collapsed nodes are set up to retain their pointer to their parent. But I think that needs to change, so that it's unnecessary to patch the pointer in the destructor. I have to look at if the parent pointer is important enough to maintain.

m-7761 commented 5 years ago

List is another big mess. It doesn't use int_val to mark its selection. Its constructors suggest it supports a textual live-variable, but it doesn't do that either. Admittedly that's not such a bad design in principal, but its live-variable is completely unused.

Probably a good fix is to write text to a text variable. I've modified live-variables so that setting one value automatically sets the others, since a lot of code was doing that. I think it would be very useful to have the list set the variable to its selection, but it would be ambiguous if its text is numeric. So it would have to have a mode to do that. It writes that to its scrollbar, if it has one. And it has a curr_line field also.

P.S. I had a random memory jogging when I remembered writing "My race" above. It occurred to me that could sound weird in the current climate of worldwide racism. I mean the "human race" (naturally) because saying species or human beings sounds like something an extraterrestrial would utter. (My brain isn't self-defensive to the extent it's thinking how words can be misconstrued in real-time. Sorry; I guess my subconscious is on a days long delay too.)

EDITED: I should add, there's an obvious bad habit in many of these classes, that goes like, whenever someone made them, they copied an existing control class, and left all of the old parts of the original class around without removing them, even when they are not used and not appropriate. It's slap-dash work like this that has no place in a library. I mean, a "library" is not something that should be developed by unseasoned programmers, since its deficits will impact all of its users. Libraries are very tricky beasts.

nigels-com commented 5 years ago

Generally speaking there is a lot of crufty, nasty and neglected code in circulation, lurking in obscure corners and long forgotten github repos. So I don't find GLUI remarkable in that sense, but for me C++ programming has moved on from this largely - more of a domain for professionals than enthusiasts. Do they even teach it in school anymore? Not so much, I guess.

I have indeed spent a lot of time in similar code bases, and been well paid to do so. Improvement is possible, but at the expense of time, commitment and transitional cost. Some more systematic test coverage would be nice, but wasn't so commonplace back in the day.

m-7761 commented 5 years ago

I took an intro to programming course in school, and proceeded from there to tear through the text books for every programming course up to the graduate classes before the end of the semester. It was a C++ course. I don't think you can learn programming in a school. You can learn some of the logical aspects of computer science, which is more akin to physics and theory, but schools never really taught programming. My mother graduated with a programming degree and doesn't understand the first thing about programming, literally. She worked in the schools registrar office with her degree, never really programming. So I think programming is something perhaps the average person is not well suited to. I find most code completely unreadable personally, and I think that's because a lot of programmers are linear thinkers, that look at code like its a book you read one word at a time, left to right, top to bottom, with spaces. If that's what you think programming is you are better off writing small one-off scripts for personal projects than doing harm in the world of programming. C++ lends itself to structural systems. The code should look more like an animal with arms and legs than the text of a novel. If you can't see the animal when you look at it, and just see words, then you are in the wrong line of work. Code is like a picture, it should have geometric elements. If you are looking at individual words you're not seeing the structure. That's probably why the noncommercial software world is so poor, and why the commercial software world probably keeps its code to itself, not because it really believes in its value, but because its too embarrassed to show the world how its sausage is made.

m-7761 commented 5 years ago

List again baffles. It has spacebar_mouse_click=false but doesn't fire a callback when keyboard navigation is used. I think it copied this from the text input box in the way I described bad coding practices yesterday. Still, it's also strange in that clicking fires the callback, but it has a secondary callback system "associated_object" that can be configured to only respond to double-clicking that FileBrowser uses for double-clicking.

There aren't a lot of comments of programmer intent in the library. I don't know if programmers felt their code was airtight to demonstrate its own intent, but there are comments of other variety. User comments or an up-to-date manual would go a long way. There's a DOC file I can't open. I can open a DOCX file I downloaded yesterday. DOC is not a format that Windows can read out of the box. I think it needs Microsoft Word or Office or something. Maybe OpenOffice.org, but I don't think it's a good candidate for documentation. List is not in the original documentation, so I assume it arrived later.

I think the original documentation is quite good. Perhaps after some housecleaning someone would find it worthwhile to prepare a new documentation based on its format. I can't volunteer that myself, but can offer consulting (answer any questions) on the subject. I think maintainers should take on this kind of responsibility. When you offer to maintain something it's not an honorary title.

If I can give this a hundred hours of my time on merit, I think updated documentation is not too much to take on for someone to provide something in return. It might be a more attractive library after. Better documentation could inspire outside involvement. Though I can't remember how I first came across GLUI myself. If I could find it, it must have some visibility.

m-7761 commented 5 years ago

Scrollbar doesn't execute_callback if it has an associated_object but I think these are not mutually exclusive. I'm going to change it to match the List behavior so at least they're not inconsistent uses of these terms.

I think it's a little strange that FileBrowser can fire a callback on double-click but the user callback cannot. That's why I think there needs to be more arguments in the callbacks, so there can be a more than one event, and so a callback can be given a reason. Or else an end-user really doesn't know why they are receiving a callback or how to respond to it.

nigels-com commented 5 years ago

DOC is not a format that Windows can read out of the box.

Both Mac Pages and LibreOffice can read the glui_manual.doc. You had me worried that maybe git had text-mangled it or something, but it seems fine. The PDF also seems happy to open, thankfully.

Markdown or Doxygen both seem like possibilities, but someone else would need to be driving force on that.

m-7761 commented 5 years ago

Doxygen is not used for professional documentation. It just collates comments in code. I'm not fan of Adobe, but the PDF works fine I think. It just needs to be remade, if that's possible without buying Adobe Acrobat Reader. Of course you can download software, but documentation should just open on any system. (Not everyone has Adobe, but I'm sure that's 1 out of a thousand versus 1 out of 2 for DOC.)

The DOC file opens into WordPad but it's like garbled binary data. I'm unsure what's in it. If it's newer or older or identical to the PDF.

EDITED: If we are talking not using PDF then HTML is always the way. EDITED: Come to think of it, I think major browsers all have built-in PDF readers.

m-7761 commented 5 years ago

I think I got the wrong idea about post_update_main_gfx possibly from some controls that may also be using it the wrong way.

I kind of thought it was a central glutPostRedisplay method, but it seems to be completely unrelated to the UI objects. I started to wonder why there was a "glut_window_id" and a "main_gfx_window_id" too. I now have to go back and check every instance of this method along with "redraw_window", "redraw", "should_redraw_now", "refresh" and figure out what is the correct behavior.

I think main_gfx_window_id is probably a bad design pattern. I'm going to have to change the names of some new methods I've added since I named them to correspond to this.

I think it refers to a hypothetical viewport or canvas that has nothing to do with GLUI. And I don't understand why GLUI would need to communicate with it if so. Because 1) the user program is in control of it, and is consuming GLUI, and 2) who is to say a GLUI window is not catering to multiple viewports or other display windows.

It's presumptuous. And execute_callback switches to this window. That makes no sense to me. The user ought to be responding to the callback. And if it switches to a window, it should be the window of its control's.

m-7761 commented 5 years ago

P.S. This is taking me longer than I wished. Of course I'm doing extra work. I'm going to take a very brief break to work on some other things. Below is a look at how the new glui.h file will look as I imagine it. Please ignore the "xcv" stuff. It's just how I've renamed the project space momentarily. Imagine it says "glui".

xcv.zip

This way accomplishes what I described in 2017 as a way to not pull OpenGL and GLUT headers into the project through glui.h, but it does it by switching to a new header, glui.hpp, instead that doesn't have the legacy requirement of doing so.

The glui_internal.h headers are now including GLUT but do not include much else. I will probably convert one into a precompiled-header when I start compiling this stuff (EDITED: It compiles already, except not this header for some typos. I'm just not yet to the iterative testing phase.) In that case every CPP files should just include one of these standard instead of mixing and matching them. I'm not planning to rewrite the Example CPP files, so that they will use/demonstrate this glui.h wrap-around glui.hpp approach.

m-7761 commented 5 years ago

Follow-up: All post_update_main_gfx calls really go through output_live so how this shook out was to make these legacy methods. Three new callbacks are provided: output_callback, canvas_callback, and active_callback. The old functionality is a special case of output_callback and so wraps around it. The third is the default callback that execute_callback uses for the active control. These are just straight data members. The other is for drawing over the UI window.

The concept of the "gfx" window had to be replaced with the "esc" window, so that the data situation doesn't really change. The decision to wrap execute_callback in the "gfx" window is replaced by a bool value set by set_main_esc_window that is really just to get the old set_main_gfx_window behavior. The "esc" window is given focus when Esc is typed, which emulates Win32 dialogs. I need this myself.

inline void UI::post_update_main_gfx();
{
    GLUT::_post_func(_main_esc_window_id,GLUT::DISPLAY);
}
static void glui_update_main_gfx(Control *c)
{
    c->ui->post_update_main_gfx();
}   
inline void UI::set_main_gfx_window(int window_id)
{
    set_main_esc_window(window_id,true);

    output_callback = glui_update_main_gfx;
}
m-7761 commented 5 years ago

List down arrow key is off-by-1. Multiple key/mouse handlers return true when they should false. Control default implementations returns false when should return true.

m-7761 commented 5 years ago
void (*glut_mouse_CB)(int, int, int, int);
void (*glut_keyboard_CB)(unsigned char, int, int);
void (*glut_special_CB)(int, int, int);
void (*glut_reshape_CB)(int, int);

I think removing these is a mistake in the current code. For reshaping I can't see another way for user code to respond to dragging the frame of an OS window. The others seem to be fallbacks for when the control's handlers return true.

I'm adding a UI* to their signature so they are more user-friendly. Maybe UI& would be better long term. It doesn't look like they worked like set_glutReshapeFunc. I think they were callbacks assigned like regular pointers.

EDITED: I'm changing the names to mouse_callback, etc. I was going to add mouse-wheel support before getting stuck in this. It's not strictly portable since user code must call UI::wheel itself, somehow.

m-7761 commented 5 years ago

Scrollbar down key input is written for horizontal; scrolls up.

m-7761 commented 5 years ago

List doesn't work with spacebar via spacebar_mouse_click. Or rather the simulated click is misinterpreted.