Messages

31

Feature requests / Assertions in sf::Image::getPixel()

« on: November 16, 2015, 08:34:02 pm »

This is a tiny thing, but a helpful one I think.
Recently more and more assertions have been added to the SFML code (see the new audio reader/writer files) and I think it's an awesome thing from a developers point of view. I have been working with the sf::Image class recently and I think the sf::Image::getPixel(x, y) method could really benefit for a debug assertion that checks if x and y are within range. Speed is what count here, so I totaly see the reason to not do bounds checks in release mode, but I think assertions in debug mode can be really helpful. The documentation only specifies that x and y are the "coordinate of pixel to get" but it doesn't specify in which range. Could be (0, size] or [0, size). (It's the first one by the way ) It took me some trial & error and code reading, that I want to save the next user. Thats way I'm suggesting the assertions and mentioning the expected range in the documentation.

32

Feature requests / Re: SoundRecorder improvments

« on: November 16, 2015, 08:14:00 pm »

Sorry for the long absence. I know it's bad to first be pushy and then don't get back at all myself, but I started a new job and had some pretty big things going on in my personal life, so I literally had no time at all.
I will split the code into seperate pull request. One for stereo recording and one for the pure virtual function call exeption. The other change is not wanted as I understand it, eventhough I still think it would make setDevice() a lot more useable and friendly to use!

As for your comment. I think it's highly unlikely that we will support more that recording 2 channels, because OpenAL doesn't, but I can change the interface to this:

void setChannelCount(unsigned int channelCount);
unsigned int getChannelCount() const;

But what should I do if the user sets a channelCount > 2 ? Fallback to 2? Return an error?

I will try to send the pull requests soon^TM

33

Feature requests / Re: SoundRecorder improvments

« on: October 06, 2015, 04:43:15 pm »

No comment at all?

34

Feature requests / SoundRecorder improvments

« on: October 02, 2015, 03:57:32 pm »

Hello everybody!
I have been using SoundRecorder quiet a bit in the past for different things and I noticed a couple things that need improvment. I have been using some of these improvments in my own code for a while, but I found the time now to make some clean commits.
So first up I noticed an error when you destroy a sf::SoundBufferRecorder while it's recording. You get a

Code: [Select]

pure virtual method called
terminate called without an active exceptionerror in the console when doing so. After diging into the code I think I found the answer. It's a threading issue. SoundBufferRecorder::onProcessSamples overwrittes the pure virtual function SoundRecorder::onProcessSamples which is called by the audio capture thread. When SoundBufferRecorder is destroyed it's method gets deleted, before the thread stops using it. Calling stop() in SoundBufferRecorders destructor fixes this problem. I think we should notice in a red box in the tutorials that a derived class has to call stop in it's destructor, since there is already a section on threading issues. Here is my fix.

Secondly I would like to suggest a new feature: Stereo recording. I have used it for quiet some time now and it comes in really handy. I saw there are also a couple threads on the forum with people requesting it. The answers suggest that it will be implemented later on. Here is my commit.

The last thing is something that I have implemented myself, but using it I noticed it would be better to do it differently. Right now if you call setDevice with an invalid device name while you are already recording the recording stops. Of course sometimes that's not what you want. So I think it would be a better behaviour to print an error message and fall back to device already in use if the one you specified is not available. Think of an application like VOIP or a sound installation where you want to make sure that there are always samples available. The commit can be found here. This commit also fixes another small issue when the method is called with an empty string.

So tell me what you think. The commits are ready to go and I'll send a PR once I heard some feedback.

35

General discussions / Re: Multi monitor support

« on: October 02, 2015, 02:36:55 pm »

First of all I'd like to say that it's great to see an active discussion on this topic. I'm not a library designer but I'll still throw in some of my thoughts.

I like the idea that you can use getAll() and a range based for loop to iterate over the screens. Although I can't think of many use cases where you'd want to use STL algorithms on screens. Never the less I think it's good practice to support them.

getPrimary() is also a good idea, because right now there are already some spots in the code where sf::Screens::get(0) is used to get the primary screen. getPrimary() is definitly more expressive and that's what people will use most of the time, because most people have one screen.

I think the data should be cached internally. It makes screens a lighter resource. And when you think about it screen changes are unlikely to happen while the program runs. Usually you set your screens up, before you start a program. So if a screen change event occurs you could simply set a flag and recompute the internal screen data at the next use. So we have a resource that doesn't change very often and we can easily be notified about change once we have screen events. These are ideal conditions for caching. If you only hand out copys or references you also don't have problems with invalid pointers.

To keep a getByIndex() function would be helpful I think. I can think of cases where users want to get a screen from it's index (for example get the screen corresponding to a video mode), instead of copying the whole vector and the only pick one element. Also we do that internally when switching to fullscreen.

There are two other things I'd like to discuss. First is I'd like to suggest a sf::Screen::contains(sf::Vector2) function. I can think of use cases where the user wants a window to go fullscreen on the monitor it's currently on. Right now there is no way to do it. Also this function would be usefull here (construct a window from handle) to get the bits per pixel of the correct monitor (right now it just takes the primary one).
The other thing is a commit I made, that removes the restriction that you can only have one fullscreen window at a time. That made sense when we didn't have multi screen support, but I think we can savely remove it now. I made a seperate commit, so it's easier to review. I'll squash them once it's reviewed.

36

General discussions / Re: Multi monitor support

« on: September 29, 2015, 03:42:22 pm »

Thanks for the review and the detailed feedback!

Accessor structs look like a nifty little pattern. I've never heared about those. Thanks for the tip! I went ahead made the constructor private and added a ScreenAccess struct. I put it into the Screen.hpp because having two seperate files for such a small thing seemed like overkill to me. (I kept the constructor because I think an object should always be in a valid state and not have any bogus data, even if it is overwritten directly afterwards.)

The coexistence of get() and getCount() is not really nice indeed.

I agree with that. So what's you verdict on this? Leave it as it is?

• That was for developing. I took it out and redirected any errors to sf::err().
• Replaced.
• Done.
• Done.
• Well I thought that that would be the best behaviour in case of an error. But it could definitly be considert for an excetion.
• Do you mean here? I rewrote it a bit, but I don't really know how else to put it. What's not clear?
• I went over the style guide again and I hope I didn't miss anything.
• Same.
• Hmm I admit its pretty big, but the problem is that some calls depend on each other and some calls have to be made in a certain order, so it's not that easy to separate them. I documented everything to make it as clear as possible. But I'll look into it again.
• I took the code from sf::VideoMode::getFullscreenModes(), but yours is more elegant and efficient. Done

I tested the API on Windows 10 with two screens, it works great. Thanks a lot for your contribution.

Thank you! Thats great to hear.

I rewrote the example in the documentation based on your gist.
So here is the new commit.

I'd like to know what others think about the sorting. Should it be removed or not?

I also made another commit that removes the limitation of being able to create only one fullscreen window. So you can now create a fullscreen windows on each of your screens at the same time! I left it in a seperate commit because it is easier to review this way. I'll squash them once it is reviewed.

37

SFML website / Re: Recording audio tutorial update

« on: September 27, 2015, 06:14:02 pm »

I sent a pull request, which is ready for proof reading.
It would be nice if somebody could do the french translation once it's merged, because my french is really bad

38

Window / Re: Cross-platform detect monitor frequency ?

« on: September 26, 2015, 11:35:34 pm »

Like zsbzsb mentioned I am working on multi screen support. It also adds the ability to get the screens refresh rate for every screen like this: sf::Screen::get(id).refreshRate. Currently there is only a windows implementation. But I would be happy if you could test it and see if it works for you. My branch: https://github.com/Foaly/SFML/tree/multi-monitor

39

General discussions / Re: Multi monitor support

« on: September 26, 2015, 11:29:43 pm »

Well the public constructor is there mostly for convenience and better encapsulation in sf::ScreenImpl. The ensureScreenListInitilized() is implemented as a function in an anonymous namespace in src/SFML/Window/Win32/ScreenImpl.cpp. So if sf::Screens constructor was private I would have to move ensureScreenListInitilized() to a private method in sf::ScreenImpl and make the two classes friends. priv::getDisplayDeviceFromId() needs access to ensureScreenListInitilized so I would have to move that into sf::ScreenImpl too. But it's a windows specific function, so I don't know if that's very clean... Do you think that would be better? I agree that it doesn't make much sense to let users create their own sf::Screen objects.

I think sf::Screen::count() makes sense and is expressive, but if it's inconsistent with the rest of the API I will change it. Do other think the same?

I'll removed all the unnecessary sf:: I could find. Tell me if I missed any.

Awesome! I'll be glad to hear some feedback.

Here is the new commit.

40

General discussions / Re: Multi monitor support

« on: September 26, 2015, 06:42:35 pm »

Alright I found some time to implement the discussed changes. New commit.
The deprecation macro and std::wstring are gone. I added quiet a bit of documentation (mostly about the sf::Screen and sf::VideoMode class). If someone could proof read it, that would be great!
It finally made click in my head and I understood that the sf::Screen object returned by SFML through get() is protected by being a const reference. So there is no need have a seperate constructor. I removed it like Hiura suggested. Something was scrambly there in my head
I made a seperate commit for moving sf::Rect, but it didn't go as expected. Eventough I used git mv include/SFML/Graphics/Rect.hpp include/SFML/System/Rect.hpp and created a new file, it still looks like I edited the files content on GitHub. Apparently git doesn't like it if you move and create a file with the same name. Also GitHub doesn't show a moved files history. If I use git log --follow include/SFML/System/Rect.hpp the history is all there, but on GitHub there is nothing to see. I asked on SO, but there seems to be no solution.
About the sorting: I think we should provide a consistent order in which the monitors are returned. Otherwise it will be dependend on the OS or even be at random, which I don't think is very clean. Since most setups on a desk are side by side, I thought sorting them from left to right would be the way to go. But of course I can also add a check for sorting them top to bottom incase they have the same x value.

If some more people could do some testing and check if everything works like they'd expect, that would be awesome. Even if you only have 1 monitor or if you have a DPI screen or a win7 or a TV/projector connected as a second screen. Anything would help

41

SFML website / Re: Recording audio tutorial update

« on: September 25, 2015, 03:42:31 pm »

Ok cool!
I'll try to find some time for it on the weekend.

42

SFML website / Recording audio tutorial update

« on: September 25, 2015, 03:21:38 pm »

Hello.
I was having a look at the the Recording audio tutorial and I noticed something wrong. In the section Custom recording there is a big red box stating that you the rate at which onProcessSamples is being called, is hard coded into SFML. To my knowledge this is configurable since 2.2 (this commit).
Also I noticed that the tutorial doesn't mention that you are able to configure the input device used for recording anywhere. I thought since there is already a dedicated tutorial for sound recording, we might as well mention it. I think it's a very handy feature (not because I wrote it, but because I have used it quiet often )
So what I am asking is would anybody mind if I removed the box and wrote a section on selecting the input device?

43

SFML development / Re: API deprecation model

« on: September 18, 2015, 08:21:57 pm »

[deprecated] is only available in C++14 as far as I know.

@Mario: Thanks! Good catch. I'll report that to the openFrameworks developers!

edit: whoops I was to quick This article seems to explain it quiet well. According to the article it is already built in Clang 3.4 and GCC 4.9

44

General discussions / Re: Multi monitor support

« on: September 18, 2015, 05:01:45 pm »

Thanks for the review and the feedback! It's nice to hear my work is appreciated

I must have missed that there was a separate discussion for that. I left a post with my code there. Of course that belongs in a separate commit! I'll remove it, leave a comment in the doc and wait until something proper has been added.

Since sf::IntRect is needed for the sf::Screen class, I thought it would be the best to move Rect.hpp into the System class. It seemed to be most fitting since sf::Vector is already there. But of course I will rework it into a separate commit using git mv, so the history isn't lost.
I know #warning isn't a standard preprocessor directive, but all the available compilers support it and I didn't know any other way to issue a compile time warning.

I know a constructor with that many arguments is ugly and not expressive, but I didn't really know how else to do it. Do you have a suggestion? I can make it private again, but the way I understood Laurent it's OK if the user can construct and manipulate a sf::Screen instance as they wish as long as the ones returned by SFML are immutable.

What do you suggest for a consistent naming? The sf::Shape classes have getPointCount(), sf::SoundBuffer has getSampleCount(), etc. but I think that was chosen because it's not clear what sf::Shape::count() or sf::SoundBuffer::count() is supposed return. For sf::Screen::count() it's quiet clear I'd say. But of course I am open for a name that fits better into the consisting API. Also returning std::size_t would be a more consistent right?

I haven't written all of the documentation yet, because I didn't want to rewrite everything while the API is still changing. The idea behind the ID is that you can find the screen belonging to a videomode (to get the screen from mode and to verify the screen supports the mode). The ID's are continuous in range [0 .. count()[. The primary screen is always at index [0] and the rest is sorted from left to right in virtual screen space. I will add some documentation about that. What do others think is index the better name for this?

About const std::wstring ... That used to be a reference, so the const made sense back then. I'll remove it. Of course std::wstring doesn't make sense in the public API, since there is sf::String for that. It's also a unfinished left over and I'll change that as well.

45

SFML development / Re: API deprecation model

« on: September 18, 2015, 02:01:22 pm »

I needed this functionality for the multi monitor support and I didn't know this had been discussed or that this thread existed.
So i just went ahead and implemented it myself. Here is what the code looks like:

////////////////////////////////////////////////////////////
// Cross-platform deprecation warning
////////////////////////////////////////////////////////////
#ifdef __GNUC__
        // clang also has this defined. deprecated(message) is only for gcc >= 4.5
        #if (__GNUC__ >= 4) && (__GNUC_MINOR__ >= 5)
        #define SFML_DEPRECATED(message, func) func __attribute__ ((deprecated(message)))
    #else
        #define SFML_DEPRECATED(message, func) func __attribute__ ((deprecated))
    #endif
#elif defined(_MSC_VER)
        #define SFML_DEPRECATED(message, func) __declspec(deprecated(message)) func
#else
        #warning WARNING: This compiler does not support SFML_DEPRECATED
        #define SFML_DEPRECATED(message, func) func
#endif

(Github for better highlighting)

It is basically a simplified version of the openFrameworks deprecation macro. I have been using it in the past and it worked very good. What I liked especially about it is that it not only emits a warning, but also prints a message. Of course the message has to be set to something helpful that tells the user what to use instead. Example what it could look like:
/home/WAGE06/playground/Foaly_SFML/Source/src/SFML/Window/Unix/WindowImplX11.cpp|1134|warning: ‘static sf::VideoMode sf::VideoMode::getDesktopMode()’ is deprecated (declared at /home/playground/Foaly_SFML/Source/include/SFML/Window/VideoMode.hpp:70): sf::VideoMode::getDesktopMode() is deprected, use sf::Screen::get(0).getDesktopMode() instead. [-Wdeprecated-declarations]

I have used it with variables, types and functions on gcc and experienced no problems. According to this it should also work with enums but I haven't tested it.