0

I have the following code which reads from a given input file into and then into struct I have made.

    OFFFile ReadOFFFile(OFFFile fileData, FILE* srcFile)
{
    int nvert, nfaces;
    fscanf(srcFile, "%s\n");
    fscanf(srcFile, "%d %d %s\n", &nvert, &nfaces);
    fileData.nvert = nvert;
    fileData.nfaces = nfaces;
    
    fileData.vertices = (int *) malloc(fileData.nvert * sizeof(float));
    fileData.triFaces = (int *) malloc(fileData.nfaces * sizeof(int));

    // Print to check correct size was allocated
    printf("%d\n", (fileData.nvert * sizeof(float)));
    printf("%d\n", (fileData.nfaces * sizeof(int)));

    int i;
    float ftemp1, ftemp2, ftemp3;
    int itemp1, itemp2, itemp3;

    fscanf(srcFile, "%f", &ftemp1);
    printf("%lf", ftemp1);
    fscanf(srcFile, "%f", &ftemp2);
//    fscanf(srcFile, " %lf", &ftemp3);

/*    for (i = 0; i < nvert; ++i)
    {
        fscanf(srcFile, "%f %f %f\n", &ftemp1, &ftemp2, &ftemp3);
        fileData.vertices[i].x = ftemp1;
        fileData.vertices[i].y = ftemp2;
        fileData.vertices[i].z = ftemp3;
    }
*/
    return(fileData);
}

The problem I am having is with the whole last section that is currently in quotes (The 2 fscanf lines above it are me attempting to test). If I have just one float being read it works fine, but when I add the second or third the whole function fails to even run, although it still compiles. I believe it to be caused by the negative sign in the input, but I don't know how I can fix it.

The data is in the form

OFF
4000 7000 0
0.8267261981964111 -1.8508968353271484 0.6781123280525208
0.7865174412727356 -1.8490413427352905 0.7289819121360779

With the floats continuing on for 4000 lines (hence for loop). These are the structs I have made

typedef struct
{
    float x;
    float y;
    float z;
} Point3D;

typedef struct
{
    int face1;
    int face2;
    int face3;
} triFace;

typedef struct
{
    int nvert;
    int nfaces;
    Point3D *vertices;
    triFace *triFaces;
} OFFFile;

Text dump of another file with a lot less lines, also does not work in the for loop. Only using this for testing. https://justpaste.it/9ohcc

BorisOZ
  • 13
  • 1
  • 1
    Consider _not_ using `scanf` family for anything but the most simple input objects. `fgets()` `strtok()` and `strtod()` will serve better here I believe. – ryyker Feb 24 '21 at 14:34
  • Welcome to StackOverflow! Please take the [tour] and read "[ask]". -- In the first two `fscanf()` the arguments don't match the format string. Please raise your warning level to the maximum, and consider all warnings to be errors. – the busybee Feb 24 '21 at 14:34
  • 1
    *"the whole function fails to even run"* - How do you know? Please produce a [mre] – klutt Feb 24 '21 at 14:35
  • 1
    *"I believe it to be caused by the negative sign in the input"* - Did you try to remove the minus sign in the input file to confirm or falsify your theory? – klutt Feb 24 '21 at 14:36
  • Those `\n` characters in your `fscanf` calls suggest that you believe `fscanf` will be following and honoring the line-based format of your data file. But it won't. This may or may not be a problem for you. – Steve Summit Feb 24 '21 at 14:41
  • 1
    It turns out that `scanf` and `fscanf`, despite their superficial simplicity, are *very* difficult to use correctly. You may find it easier in the long run to abandon them. If you stick with `fscanf` here, you will definitely want to check its return value. If it does not return the value 1, 2, or 3, according to the number of values you asked it to convert, something has gone wrong: either your code is wrong, or the input file is malformed. – Steve Summit Feb 24 '21 at 14:44
  • Having your `ReadOFFFile()` function both accept and return an `OFFFile` isn't the best way to do it, especially if the `OFFFile` structure is large. – Steve Summit Feb 24 '21 at 14:48
  • The whole function fails to run, as in when the function is called my program exits without even running the first line and without running anything called after it in the main body. Removing the minus does seem to fix it but that's of course not a solution I'm able to use. I will have a go with using fgets instead and see how that goes. I assume what you are getting at @SteveSummit is that I should use a pointer? Haven't used C in a while so forgot when I should be using them – BorisOZ Feb 24 '21 at 14:53
  • If the compiler isn't warning you about `fscanf(srcFile, "%s\n");`, then you need to turn up your compiler warnings. What are you hoping that line of code does? – William Pursell Feb 24 '21 at 14:59
  • It is now warning me since I have turned it up, it is meant to just skip that line because the file always starts with that line although I have realised it is a terrible way to do it. – BorisOZ Feb 24 '21 at 15:04
  • Similarly with `fscanf(srcFile, "%d %d %s\n", &nvert, &nfaces);` If you are not going to pass an address, you need to tell scanf that you want to discard the data. Something like `fscanf(srcFile, "%d %d %*s\n", &nvert, &nfaces);` (note the `*`) – William Pursell Feb 24 '21 at 15:10

3 Answers3

0

The line:

fscanf(srcFile, "%s\n");

is invoking undefined behavior. The compiler should warn you about that. Once you've invoked UB, there's no point in speculating further about what is happening.

It's not clear to me what you intended that line to do, but if you use %s in a scanf, you need to give it a valid place to write data. You should always check the value returned by scanf to ensure that you have actually read some data, and you should never use "%s" without a width modifier. Perhaps you want something like:

char buf[256];
if( fscanf(srcFile, "%255s ", buf) == 1 ){
        /* Do something with the string in buf */
}

From your comment, it seems that you are intending to use that scanf to skip a line. I strongly recommend using a while(fgetc) loop instead of scanf to do that. If you do want to use scanf, you could try something like fscanf(srcFile, "%*s\n"), but beware that it will stop at the first whitespace, and not necessarily consume an entire line. You could also do fscanf(srcFile, "%*[^\n]%*c"); to consume the line, but you are really better off using a fgetc in a while loop.

William Pursell
  • 204,365
  • 48
  • 270
  • 300
  • I avoid `scanf()` like it's bubonic plague (yeah, I shouldn't cast aspersions about the plague like that....) but `"%s255 "` looks wrong. Maybe `"%255s "`? And that trailing space also bothers me. – Andrew Henle Feb 24 '21 at 15:09
  • 1
    @AndrewHenle Thanks for catching that. Yes, I also advise avoiding scanf. In recent years I've tried to get over my natural repulsion to it, and it does seem to have some value. But for the most part, it seems that any use case for which scanf is appropriate, C is the wrong language! – William Pursell Feb 24 '21 at 15:12
  • The trailing space definitely warrants a comment. For the most part, I'm using that since the OP also used trailing whitespace. But I don't think this scanf does what the OP wants it to do. (But I'm not sure what the desired behavior is!) – William Pursell Feb 24 '21 at 15:13
  • That first `fscanf` call is probably intended to skip the "`OFF`" on the first line of the file. Another way to do that would be `%*s`. – Steve Summit Feb 24 '21 at 15:40
  • @WilliamPursell I have definitely turned into a crank on this subject, but I firmly believe that the only use for `scanf` is for beginning programmers to get `int` values into their programs, so that they can do things like print NxN boxes made of `*` characters, during the first 2-3 weeks that they're learning C. After that they need to aggressively wean themselves off of all uses of `scanf`, never to use it again. `scanf` is square training wheels. But once you're using it for dirt-simple things, it's almost impossible not to get trapped into trying to use it for more complicated things. – Steve Summit Feb 24 '21 at 15:43
  • @SteveSummit unfortunately it's what I had to use last year when I last studied C and we were not allowed to use anything else hence I thought it was meant to be better, obviously this is not the case so I will instead try and unlearn it. Is there a simple way to skip the OFF line like you have shown that doesn't use fscanf? – BorisOZ Feb 24 '21 at 15:52
  • @BorisOZ I understand, and I don't blame you, I blame your instructor. See the answer I'm about to post. – Steve Summit Feb 24 '21 at 15:56
0

Addressing title question:

"How do I read multiple floats from one line of a file"

...with suggestions for a non-scanf() approach.

Assuming the file is opened, (and a file pointer) fp is obtained ) , the first two lines are already handled, and values into ints, say the lines value is converted to int lines;

And given your struct definition (modified to use double to accommodate type compatibility in code below):

typedef struct
{
    double x;
    double y;
    double z;
} Point3D; 

In a function somewhere here is one way to parse the contents of each data line into the 3 respective struct values using fgets(), strtok() and strtod():

char delim[] = " \n";
char *tok = NULL;
char newLine[100] = {0};
Point3D *point = calloc(lines, sizeof(*point));
if(point)
{
    int i = 0;
    while(fgets(newLine, sizeof newLine, fp))
    {
        tok = strtok(newLine, delim);
        if(tok)
        {
            if(parseDbl(tok, &point[i].x))
            {
                tok = strtok(NULL, delim);
                if(tok)
                {
                    if(parseDbl(tok, &point[i].y))
                    {
                        tok = strtok(NULL, delim);
                        if(tok)
                        {
                            if(!parseDbl(tok, &point[i].z))
                            {
                                ;//handle error
                            }else ;//continue
                        }else ;//handle error
                    }else ;//handle error
                }else ;//handle error
            }else ;//handle error
        }else ;//handle error 
        i++;//increment for next read
    }//end of while
}else ;//handle error  

Where parseDbl is defined as:

bool parseDbl(const char *str, double *val)
{
    char *temp = NULL;
    bool rc = true;
    errno = 0;
    *val = strtod(str, &temp);

    if (temp == str)
        rc = false;

    return rc;
}       
ryyker
  • 22,849
  • 3
  • 43
  • 87
  • Points for advocating `fgets` over `fscanf`, but there has *got* to be a better way than using six nested `if`'s! – Steve Summit Feb 24 '21 at 15:46
  • @SteveSummit - LOL, Yes I agree, in production code I surround each call with a macro that upon error takes execution flow to the bottom of function, cleans up memory, closes open files logs error and returns. But for this I just wanted to show a working segment. (It _is_ ugly) – ryyker Feb 24 '21 at 15:50
0

Your main problem is the first line in the readOFFFile function:

fscanf(srcFile, "%s\n");

This tries to read a string (presumably the string OFF on the first line of the file), but you don't give fscanf any place to store the string, so it crashes. (As an aside, your compiler really should have warned you about this problem. If it didn't, it's old-fashioned, and there are lots of easy mistakes that it's probably not going to warn you about, and learning C is going to be much harder than it ought to be. Or perhaps you just need to find an option flag or checkbox to enable more warnings.)

You can tell fscanf to read and discard something, without storing it anywhere, using the * modifier. Here's a modified version of your program, that works for me.

void ReadOFFFile(OFFFile *fileData, FILE* srcFile)
{
    fscanf(srcFile, "%*s");
    if(fscanf(srcFile, "%d %d %*s", &fileData->nvert, &fileData->nfaces) != 2) {
        exit(1);
    }
    
    fileData->vertices = malloc(fileData->nvert * sizeof(Point3D));
    fileData->triFaces = malloc(fileData->nfaces * sizeof(triFace));

    int i;

    for (i = 0; i < fileData->nvert; ++i)
    {
        if(fscanf(srcFile, "%f %f %f", &fileData->vertices[i].x,
                                       &fileData->vertices[i].y,
                                       &fileData->vertices[i].z) != 3) {
            exit(1);
        }
    }
}

I have made a few other changes. The other fscanf call, that reads three values but only stores two, also needs a * modifier. I check the return value of fscanf to catch errors (via a crude exit) if the input is not as expected. I got rid of the \n characters in the fscanf calls, since they're not necessary, and potentially misleading. I got rid of some unnecessary temporary variables, and I had the readOFFFile function accept a pointer to an OFFFile structure to fill in, rather than passing and returning it.

Here is the main program I tested it with:

int main()
{
    OFFFile fd;
    FILE *fp = fopen("dat", "r");
    ReadOFFFile(&fd, fp);
    for (int i = 0; i < fd.nvert; ++i)
        printf("%f %f %f\n", fd.vertices[i].x, fd.vertices[i].y, fd.vertices[i].z);
}

This is still an incomplete program: there are several more places where you need to check for errors (opening the file, calling malloc, etc.), and when you do detect an error, you need to at least print a useful error message before exiting or whatever.

One more thing. As I mentioned, those \n characters you had in the fscanf format strings were unnecessary and misleading. To illustrate what I mean, once you get the program working, have it try to read a data file like this:

OFF 2 0
0 0.8267261981964111
-1.8508968353271484 0.6781123280525208
0.7865174412727356 -1.8490413427352905 0.7289819121360779

Totally malformed, but the program reads it without complaint! This is one reason (one of several dozen reasons, actually) why the scanf family of functions is basically useless for most things. These functions claim to "scan formatted data", but their definition of "formatted" is quite loose, in that they actually read free-form input, generally without any regard for line boundaries.

For some advice on graduating beyond scanf and using better, more reliable methods for reading input, see this question. See also this section and this section in some online C programming course notes.

Steve Summit
  • 45,437
  • 7
  • 70
  • 103
  • Thanks for that, it seems to be mostly working now. I think there is a malloc issue though, mostly my bad from the start. Because vertices stores 3 floats (it being a Point3D) I should've also had a ` * 3 ` in there. I also removed the nvert and nfaces declaration at the start because you fixed up the first fscanf. Have also figured out how to turn on more warnings, although I'm not sure which ones are really necessary for me. I've turned almost all of them on and just do a google search if something appears that I don't understand. – BorisOZ Feb 24 '21 at 16:24
  • @BorisOZ Oh, dear. I completely missed the `malloc` problem. Rather than multiplying by 3, a better solution is to use the size of the actual structures, as I have now done in the edited answer. Even better, perhaps, is to use things like `fileData->vertices = malloc(fileData->nvert * sizeof(*fileData->vertices));`. – Steve Summit Feb 24 '21 at 16:30